This patch series were done by Vanderson and Alex originally in 2019, I
(Fei Wu) rebased them on latest upstream from:
    https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
and send out this review per Alex's request, I will continue to address
any future review comments here. As it's been a very long time and there
are lots of conflicts during rebase, it's my fault if I introduce any
problems during the process.

Alex in his v10 series has addressed part of v9 comments:
   https://www.mail-archive.com/qemu-devel@nongnu.org/msg650258.html
and I tried to figure out some others. It looks to me not all of v9
comments had made complete agreement and I didn't join that discussion
then, so please comment to this series and I will fix them up if
necessary.


Alex Bennée (5):
  accel/tcg: move profiler dev_time to tb_stats
  accel/tcg: convert profiling of restore operations to TBStats
  accel/tcg: convert profiling of code generation to TBStats
  tb-stats: reset the tracked TBs on a tb_flush
  configure: remove the final bits of --profiler support

Vanderson M. do Rosario (9):
  accel/tcg: introduce TBStatistics structure
  accel: collecting TB execution count
  accel: collecting JIT statistics
  accel: replacing part of CONFIG_PROFILER with TBStats
  accel: adding TB_JIT_TIME and full replacing CONFIG_PROFILER
  debug: add -d tb_stats to control TBStatistics collection:
  monitor: adding tb_stats hmp command
  Adding info [tb-list|tb] commands to HMP (WIP)
  tb-stats: dump hot TBs at the end of the execution

 accel/tcg/cpu-exec.c          |   6 +
 accel/tcg/meson.build         |   1 +
 accel/tcg/monitor.c           | 133 +++++--
 accel/tcg/tb-context.h        |   1 +
 accel/tcg/tb-hash.h           |   7 +
 accel/tcg/tb-maint.c          |  20 +
 accel/tcg/tb-stats.c          | 689 ++++++++++++++++++++++++++++++++++
 accel/tcg/tcg-accel-ops.c     |  15 +-
 accel/tcg/tcg-runtime.c       |   8 +
 accel/tcg/tcg-runtime.h       |   2 +
 accel/tcg/translate-all.c     | 147 ++++++--
 accel/tcg/translator.c        |  13 +
 disas.c                       |  31 +-
 hmp-commands-info.hx          |  16 +
 hmp-commands.hx               |  16 +
 include/exec/exec-all.h       |   3 +
 include/exec/gen-icount.h     |   2 +
 include/exec/tb-stats-dump.h  |  21 ++
 include/exec/tb-stats-flags.h |  32 ++
 include/exec/tb-stats.h       | 165 ++++++++
 include/monitor/hmp.h         |   3 +
 include/qemu/log-for-trace.h  |   6 +-
 include/qemu/log.h            |   3 +
 include/qemu/timer.h          |   5 +-
 include/tcg/tcg.h             |  50 ++-
 linux-user/exit.c             |   2 +
 meson.build                   |   2 -
 meson_options.txt             |   2 -
 scripts/meson-buildoptions.sh |   3 -
 softmmu/runstate.c            |  10 +-
 stubs/meson.build             |   1 +
 stubs/tb-stats.c              |  32 ++
 tcg/tcg.c                     | 224 +++--------
 util/log.c                    | 103 ++++-
 34 files changed, 1482 insertions(+), 292 deletions(-)
 create mode 100644 accel/tcg/tb-stats.c
 create mode 100644 include/exec/tb-stats-dump.h
 create mode 100644 include/exec/tb-stats-flags.h
 create mode 100644 include/exec/tb-stats.h
 create mode 100644 stubs/tb-stats.c

-- 
2.25.1

Fei Wu <fei2.wu@intel.com> writes:

> This patch series were done by Vanderson and Alex originally in 2019, I
> (Fei Wu) rebased them on latest upstream from:
>     https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
> and send out this review per Alex's request, I will continue to address
> any future review comments here. As it's been a very long time and there
> are lots of conflicts during rebase, it's my fault if I introduce any
> problems during the process.

Hi Fei,

Thanks for picking this up. I can confirm that this applies cleanly to
master and I have kicked the tyres and things still seem to work. I'm
not sure if I can provide much review on code I wrote but a few things
to point out:

  - there are a number of CI failures, mainly qatomic on 32 bit guests
    see https://gitlab.com/stsquad/qemu/-/pipelines/844857279/failures
    maybe we just disable time accounting for 32 bit hosts?

  - we need a proper solution to the invalidation of TBs so we can
    exclude them from lists (or at least not do the attempt
    translation/fail dance). Alternatively we could page out a copy of
    the TB data to a disk file when we hit a certain hotness? How would
    this interact with the jitperf support already?

  - we should add some documentation to the manual so users don't have
    to figure it all out by trail and error at the HMP command line.

  - there may be some exit cases missed because I saw some weird TB's
    with very long IR generation times.

    TB id:5 | phys:0xb5f21d00 virt:0xffffcf2f17721d00 flags:0x00000051 1 inv/2
            | exec:1889055/0 guest inst cov:1.05%
            | trans:2 ints: g:4 op:32 op_opt:26 spills:0
            | h/g (host bytes / guest insts): 56.000000
            | time to gen at 2.4GHz => code:6723.33(ns) IR:2378539.17(ns)

  - code motion in 9/14 should be folded into the first patch

Even if we can't find a solution for safely dumping TBs I think the
series without "tb-list" is still an improvement for getting rid of the
--enable-profiler and making info JIT useful by default.

Richard,

What do you think?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

On 4/22/2023 12:42 AM, Alex Bennée wrote:
> 
> Fei Wu <fei2.wu@intel.com> writes:
> 
>> This patch series were done by Vanderson and Alex originally in 2019, I
>> (Fei Wu) rebased them on latest upstream from:
>>     https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>> and send out this review per Alex's request, I will continue to address
>> any future review comments here. As it's been a very long time and there
>> are lots of conflicts during rebase, it's my fault if I introduce any
>> problems during the process.
> 
> Hi Fei,
> 
> Thanks for picking this up. I can confirm that this applies cleanly to
> master and I have kicked the tyres and things still seem to work. I'm
> not sure if I can provide much review on code I wrote but a few things
> to point out:
> 
>   - there are a number of CI failures, mainly qatomic on 32 bit guests
>     see https://gitlab.com/stsquad/qemu/-/pipelines/844857279/failures
>     maybe we just disable time accounting for 32 bit hosts?
> 
I sent out v12 series which fixes some CI failures. qatomic is not
touched yet, the current code with CONFIG_PROFILER should have the same
issue, what's the policy of 32 bit guests support on qemu?

Besides time, there are some other counters with uint64_t using qatomic
such as TCGProfile.table_op_count, we might switch to size_t instead?

>   - we need a proper solution to the invalidation of TBs so we can
>     exclude them from lists (or at least not do the attempt
>     translation/fail dance). Alternatively we could page out a copy of
>     the TB data to a disk file when we hit a certain hotness? How would
>     this interact with the jitperf support already?
> 
>   - we should add some documentation to the manual so users don't have
>     to figure it all out by trail and error at the HMP command line.
> 
added one in docs/tb-stats.txt. Some extra bits could be added to
explain the fields of the output.

>   - there may be some exit cases missed because I saw some weird TB's
>     with very long IR generation times.
> 
>     TB id:5 | phys:0xb5f21d00 virt:0xffffcf2f17721d00 flags:0x00000051 1 inv/2
>             | exec:1889055/0 guest inst cov:1.05%
>             | trans:2 ints: g:4 op:32 op_opt:26 spills:0
>             | h/g (host bytes / guest insts): 56.000000
>             | time to gen at 2.4GHz => code:6723.33(ns) IR:2378539.17(ns)
> 
Is it reproducible on your system? I didn't see it on my system, is it
possible the system events cause this?

>   - code motion in 9/14 should be folded into the first patch
> 
done.

btw, I also added a few comments on v12 series, could you please check
if they make sense?

Thanks,
Fei.

> Even if we can't find a solution for safely dumping TBs I think the
> series without "tb-list" is still an improvement for getting rid of the
> --enable-profiler and making info JIT useful by default.
> 
> Richard,
> 
> What do you think?
> 

On 5/18/23 18:16, Wu, Fei wrote:
> On 4/22/2023 12:42 AM, Alex Bennée wrote:
>>
>> Fei Wu <fei2.wu@intel.com> writes:
>>
>>> This patch series were done by Vanderson and Alex originally in 2019, I
>>> (Fei Wu) rebased them on latest upstream from:
>>>      https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>>> and send out this review per Alex's request, I will continue to address
>>> any future review comments here. As it's been a very long time and there
>>> are lots of conflicts during rebase, it's my fault if I introduce any
>>> problems during the process.
>>
>> Hi Fei,
>>
>> Thanks for picking this up. I can confirm that this applies cleanly to
>> master and I have kicked the tyres and things still seem to work. I'm
>> not sure if I can provide much review on code I wrote but a few things
>> to point out:
>>
>>    - there are a number of CI failures, mainly qatomic on 32 bit guests
>>      see https://gitlab.com/stsquad/qemu/-/pipelines/844857279/failures
>>      maybe we just disable time accounting for 32 bit hosts?
>>
> I sent out v12 series which fixes some CI failures. qatomic is not
> touched yet, the current code with CONFIG_PROFILER should have the same
> issue, what's the policy of 32 bit guests support on qemu?

They should work.

> Besides time, there are some other counters with uint64_t using qatomic
> such as TCGProfile.table_op_count, we might switch to size_t instead?

Probably.  You probably don't need to represent times as uint64_t (or time64_t), but as 
differentials for elapsed time.

We could accumulate into 'float' if you were concerned about overflowing 2^32 units.  This 
is statistics after all; we don't really need exact numbers, we need magnitude and 2-3 
digits of precision.


r~

On 4/22/2023 12:42 AM, Alex Bennée wrote:
> 
> Fei Wu <fei2.wu@intel.com> writes:
> 
>> This patch series were done by Vanderson and Alex originally in 2019, I
>> (Fei Wu) rebased them on latest upstream from:
>>     https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>> and send out this review per Alex's request, I will continue to address
>> any future review comments here. As it's been a very long time and there
>> are lots of conflicts during rebase, it's my fault if I introduce any
>> problems during the process.
> 
> Hi Fei,
> 
> Thanks for picking this up. I can confirm that this applies cleanly to
> master and I have kicked the tyres and things still seem to work. I'm
> not sure if I can provide much review on code I wrote but a few things
> to point out:
> 
Hi Alex,

There are several new files added, should I put your name as their
maintainer? Also, should I signed-off these patches or not, definitely
the original signed-offs will be kept.

Thanks,
Fei.

>   - there are a number of CI failures, mainly qatomic on 32 bit guests
>     see https://gitlab.com/stsquad/qemu/-/pipelines/844857279/failures
>     maybe we just disable time accounting for 32 bit hosts?
> 
>   - we need a proper solution to the invalidation of TBs so we can
>     exclude them from lists (or at least not do the attempt
>     translation/fail dance). Alternatively we could page out a copy of
>     the TB data to a disk file when we hit a certain hotness? How would
>     this interact with the jitperf support already?
> 
>   - we should add some documentation to the manual so users don't have
>     to figure it all out by trail and error at the HMP command line.
> 
>   - there may be some exit cases missed because I saw some weird TB's
>     with very long IR generation times.
> 
>     TB id:5 | phys:0xb5f21d00 virt:0xffffcf2f17721d00 flags:0x00000051 1 inv/2
>             | exec:1889055/0 guest inst cov:1.05%
>             | trans:2 ints: g:4 op:32 op_opt:26 spills:0
>             | h/g (host bytes / guest insts): 56.000000
>             | time to gen at 2.4GHz => code:6723.33(ns) IR:2378539.17(ns)
> 
>   - code motion in 9/14 should be folded into the first patch
> 
> Even if we can't find a solution for safely dumping TBs I think the
> series without "tb-list" is still an improvement for getting rid of the
> --enable-profiler and making info JIT useful by default.
> 
> Richard,
> 
> What do you think?
> 

"Wu, Fei" <fei2.wu@intel.com> writes:

> On 4/22/2023 12:42 AM, Alex Bennée wrote:
>> 
>> Fei Wu <fei2.wu@intel.com> writes:
>> 
>>> This patch series were done by Vanderson and Alex originally in 2019, I
>>> (Fei Wu) rebased them on latest upstream from:
>>>     https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>>> and send out this review per Alex's request, I will continue to address
>>> any future review comments here. As it's been a very long time and there
>>> are lots of conflicts during rebase, it's my fault if I introduce any
>>> problems during the process.
>> 
>> Hi Fei,
>> 
>> Thanks for picking this up. I can confirm that this applies cleanly to
>> master and I have kicked the tyres and things still seem to work. I'm
>> not sure if I can provide much review on code I wrote but a few things
>> to point out:
>> 
> Hi Alex,
>
> There are several new files added, should I put your name as their
> maintainer? Also, should I signed-off these patches or not, definitely
> the original signed-offs will be kept.

I assume they would just become part of the accel/tcg stuff rather than
be maintained as a separate subsystem.

For sign-offs you should keep the original authors and add your own.
Each s-o-b is a statement by the person working with the code that they
are "legally okay to contribute this and happy for it to go into QEMU".
So it is totally normal for work that goes through several trees before
being merged to have multiple sign-offs. When the maintainer creates
their pull request they will add theirs as well.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

On 5/12/2023 4:42 PM, Alex Bennée wrote:
> 
> "Wu, Fei" <fei2.wu@intel.com> writes:
> 
>> On 4/22/2023 12:42 AM, Alex Bennée wrote:
>>>
>>> Fei Wu <fei2.wu@intel.com> writes:
>>>
>>>> This patch series were done by Vanderson and Alex originally in 2019, I
>>>> (Fei Wu) rebased them on latest upstream from:
>>>>     https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>>>> and send out this review per Alex's request, I will continue to address
>>>> any future review comments here. As it's been a very long time and there
>>>> are lots of conflicts during rebase, it's my fault if I introduce any
>>>> problems during the process.
>>>
>>> Hi Fei,
>>>
>>> Thanks for picking this up. I can confirm that this applies cleanly to
>>> master and I have kicked the tyres and things still seem to work. I'm
>>> not sure if I can provide much review on code I wrote but a few things
>>> to point out:
>>>
>> Hi Alex,
>>
>> There are several new files added, should I put your name as their
>> maintainer? Also, should I signed-off these patches or not, definitely
>> the original signed-offs will be kept.
> 
> I assume they would just become part of the accel/tcg stuff rather than
> be maintained as a separate subsystem.
> 
ok. I see ./scripts/checkpatch.pl reports some warnings as follows:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

> For sign-offs you should keep the original authors and add your own.
> Each s-o-b is a statement by the person working with the code that they
> are "legally okay to contribute this and happy for it to go into QEMU".
> So it is totally normal for work that goes through several trees before
> being merged to have multiple sign-offs. When the maintainer creates
> their pull request they will add theirs as well.
> 
Got it, thank you.

Fei.

"Wu, Fei" <fei2.wu@intel.com> writes:

> On 5/12/2023 4:42 PM, Alex Bennée wrote:
>> 
>> "Wu, Fei" <fei2.wu@intel.com> writes:
>> 
>>> On 4/22/2023 12:42 AM, Alex Bennée wrote:
>>>>
>>>> Fei Wu <fei2.wu@intel.com> writes:
>>>>
>>>>> This patch series were done by Vanderson and Alex originally in 2019, I
>>>>> (Fei Wu) rebased them on latest upstream from:
>>>>>     https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>>>>> and send out this review per Alex's request, I will continue to address
>>>>> any future review comments here. As it's been a very long time and there
>>>>> are lots of conflicts during rebase, it's my fault if I introduce any
>>>>> problems during the process.
>>>>
>>>> Hi Fei,
>>>>
>>>> Thanks for picking this up. I can confirm that this applies cleanly to
>>>> master and I have kicked the tyres and things still seem to work. I'm
>>>> not sure if I can provide much review on code I wrote but a few things
>>>> to point out:
>>>>
>>> Hi Alex,
>>>
>>> There are several new files added, should I put your name as their
>>> maintainer? Also, should I signed-off these patches or not, definitely
>>> the original signed-offs will be kept.
>> 
>> I assume they would just become part of the accel/tcg stuff rather than
>> be maintained as a separate subsystem.
>> 
> ok. I see ./scripts/checkpatch.pl reports some warnings as follows:
>
> WARNING: added, moved or deleted file(s), does MAINTAINERS need
> updating?

All the stuff under accel/tcg should already be caught by:

Overall TCG CPUs
...
F: accel/tcg/

but I suspect the new headers will need explicit entries in the
MAINTAINERS file adding.

>
>> For sign-offs you should keep the original authors and add your own.
>> Each s-o-b is a statement by the person working with the code that they
>> are "legally okay to contribute this and happy for it to go into QEMU".
>> So it is totally normal for work that goes through several trees before
>> being merged to have multiple sign-offs. When the maintainer creates
>> their pull request they will add theirs as well.
>> 
> Got it, thank you.
>
> Fei.


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro