[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aB4qpAc2GThyGaqg@google.com>
Date: Fri, 9 May 2025 09:17:40 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ravi Bangoria <ravi.bangoria@....com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Ian Rogers <irogers@...gle.com>,
Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
linux-perf-users@...r.kernel.org, Leo Yan <leo.yan@....com>,
Stephane Eranian <eranian@...gle.com>
Subject: Re: [RFC/PATCHSET 00/11] perf mem: Add new output fields for data
source (v1)
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
Powered by blists - more mailing lists