| Message ID | 20230421132421.1617479-1-fei2.wu@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | TCG code quality tracking | expand |
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?
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.
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.
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~
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