Re: [PATCH v11 00/14] TCG code quality tracking

["Alex Bennée" <alex.bennee@linaro.org>]

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