Re: [RFC/PATCHSET 00/11] perf mem: Add new output fields for data source (v1)

[Namhyung Kim <namhyung@kernel.org>]

Hi Ravi,

On Thu, May 08, 2025 at 09:42:41AM +0530, Ravi Bangoria wrote:
> Hi Namhyung,
> 
> I feel the overall idea is good. Running few simple perf-mem commands
> on AMD works fine too. Few general feedback below.

Thanks for your review!

> 
> > The name of some new fields are the same as the corresponding sort
> > keys (mem, op, snoop) so I had to change the order whether it's
> > applied as an output field or a sort key.  Maybe it's better to name
> > them differently but I couldn't come up with better ideas.
> 
> 1) These semantic changes of the field name seems counter intuitive
>    (to me). Example:
> 
>    -F mem:
> 
>      Without patch:
> 
>      $ perf mem report -F overhead,sample,mem --stdio
>      # Overhead       Samples  Memory access
>          39.29%             1  L3 hit
>          37.50%            21  N/A
>          23.21%            13  L1 hit
> 
>      With patch:
> 
>      $ perf mem report -F overhead,sample,mem --stdio
>      #                          Memory
>      # Overhead       Samples    Other
>         100.00%            35   100.0%

Yep, that's because I split the 'mem' part to 'cache' and 'mem' because
he_mem_stat can handle up to 8 entries.  As your samples hit mostly in
the caches, you'd get the similar result when you run:

  $ perf mem report -F overhead,sample,cache --stdio

> 
>    -F 'snoop':
> 
>      Without patch:
> 
>      $ perf mem report -F overhead,sample,snoop --stdio
>      # Overhead       Samples  Snoop
>          60.71%            34  N/A
>          39.29%             1  HitM
>    
>      With patchset:
> 
>      $ perf mem report -F overhead,sample,snoop --stdio
>      #                         --- Snoop ----
>      # Overhead       Samples     HitM  Other
>         100.00%            35    39.3%  60.7%

This matches to 'Overhead' distribution without patch, right?

> 
> 2) It was not intuitive (to me:)) that perf-mem overhead is calculated
>    using sample->weight by overwriting sample->period. I also don't see
>    it documented anywhere (or did I miss it?)

I don't see the documentation and I also find it confusing.  Sometimes I
think the weight is better but sometimes not. :(  At least we could add
and option to control that (like --use-weight ?).

Also we now have 'weight' output field so users can see it, althought it
shows averages.

> 
>    perf report:
> 
>      $ perf report -F overhead,sample,period,dso --stdio
>      # Overhead  Samples   Period  Shared Object
>          80.00%       28  2800000  [kernel.kallsyms]
>           5.71%        2   200000  ld-linux-x86-64.so.2
>           5.71%        2   200000  libc.so.6
>           5.71%        2   200000  ls
>           2.86%        1   100000  libpcre2-8.so.0.11.2
> 
>    perf mem report:
> 
>      $ perf mem report -F overhead,sample,period,dso --stdio
>      # Overhead  Samples   Period  Shared Object
>          87.50%       28       49  [kernel.kallsyms]
>           3.57%        2        2  ld-linux-x86-64.so.2
>           3.57%        2        2  libc.so.6
>           3.57%        2        2  ls
>           1.79%        1        1  libpcre2-8.so.0.11.2
> 
> 3) Similarly, it was not intuitive (again, to me:)) that -F op/snoop/dtlb
>    percentages are calculated based on sample->weight.

Hmm.. ok.  Maybe better to use the original period for percentage
breakdown in the new output fields.  For examples, in the above result
you have 13 samples for L1 and 1 sample for L3 but the weight of L3
access is bigger.  But I guess users probably want to see L1 access was
dominant.

> 
> 4) I've similar recommended perf-mem command in perf-amd-ibs man page.
>    Can you please update alternate command there.
>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/Documentation/perf-amd-ibs.txt?h=v6.15-rc5#n167

Sure will do.

Thanks,
Namhyung

> 
> Please correct me if I'm missing anything.
> 
> Thanks,
> Ravi