Re: [PATCH 6/6] perf vendor events amd: Add Zen 4 memory controller events

[Ian Rogers]

On Wed, Jul 19, 2023 at 10:23 PM Sandipan Das <sandipan.das@xxxxxxx> wrote:
>
> On 7/19/2023 9:42 PM, Ian Rogers wrote:
> > On Tue, Jul 18, 2023 at 11:58 PM Sandipan Das <sandipan.das@xxxxxxx> wrote:
> >>
> >> Make the jevents parser aware of the Unified Memory Controller (UMC) PMU
> >> and add events taken from Section 8.2.1 "UMC Performance Monitor Events"
> >> of the Processor Programming Reference (PPR) for AMD Family 19h Model 11h
> >> processors. The events capture UMC command activity such as CAS, ACTIVATE,
> >> PRECHARGE etc. while the metrics derive data bus utilization and memory
> >> bandwidth out of these events.
> >>
> >> Signed-off-by: Sandipan Das <sandipan.das@xxxxxxx>
> >
> > Acked-by: Ian Rogers <irogers@xxxxxxxxxx>
> >
> >> ---
> >>  .../arch/x86/amdzen4/memory-controller.json   | 101 ++++++++++++++++++
> >>  .../arch/x86/amdzen4/recommended.json         |  84 +++++++++++++++
> >>  tools/perf/pmu-events/jevents.py              |   2 +
> >>  3 files changed, 187 insertions(+)
> >>  create mode 100644 tools/perf/pmu-events/arch/x86/amdzen4/memory-controller.json
> >>
> >> diff --git a/tools/perf/pmu-events/arch/x86/amdzen4/memory-controller.json b/tools/perf/pmu-events/arch/x86/amdzen4/memory-controller.json
> >> new file mode 100644
> >> index 000000000000..55263e5e4f69
> >> --- /dev/null
> >> +++ b/tools/perf/pmu-events/arch/x86/amdzen4/memory-controller.json
> >> @@ -0,0 +1,101 @@
> >> +[
> >> +  {
> >> +    "EventName": "umc_mem_clk",
> >> +    "PublicDescription": "Number of memory clock cycles.",
> >> +    "EventCode": "0x00",
> >> +    "PerPkg": "1",
> >> +    "Unit": "UMCPMC"
> >
> > nit: Why use UMCPMC and then rewrite to amd_umc, why not just use "amd_umc" ?
> >
>
> I followed the convention that has been historically used for AMD uncore PMUs e.g.
> the "Unit" for amd_df is "DFPMC" and for amd_l3 is "L3PMC". I do agree that its
> simpler to use the same naming so will change this. If you prefer, I can send out
> a separate patch to change the unit naming for amd_df and amd_l3.

Thanks for the explanation. I don't mind but it is nicer to have fewer
renames imo. If we get rid of one, perhaps we can get rid of them all?
Perhaps merge this and follow up with simplification.

Thanks,
Ian

> - Sandipan
>