Received-SPF: pass (google.com: domain of linux-kernel+bounces-99315-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1710133771.git.sandipan.das@amd.com> <0d18c3a93083f12481f27dcc5f5795877fa02b90.1710133771.git.sandipan.das@amd.com>
In-Reply-To: <0d18c3a93083f12481f27dcc5f5795877fa02b90.1710133771.git.sandipan.das@amd.com>
From: Ian Rogers <irogers@google.com>
Date: Mon, 11 Mar 2024 10:40:03 -0700
Message-ID: <CAP-5=fUV=6my-+z0Qc+TS2+CbKT1DqsLbpHZrZLitqpAnDd2-A@mail.gmail.com>
Subject: Re: [PATCH 1/4] perf vendor events amd: Add Zen 5 core events
To: Sandipan Das <sandipan.das@amd.com>
Cc: linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, 
	peterz@infradead.org, mingo@redhat.com, acme@kernel.org, namhyung@kernel.org, 
	mark.rutland@arm.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org, 
	adrian.hunter@intel.com, eranian@google.com, ravi.bangoria@amd.com, 
	ananth.narayan@amd.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Mar 10, 2024 at 10:23=E2=80=AFPM Sandipan Das <sandipan.das@amd.com=
> wrote:
>
> Add core events taken from Section 1.4 "Core Performance Monitor
> Counters" of the Performance Monitor Counters for AMD Family 1Ah Model
> 00h-0Fh Processors document available at the link below. This
> constitutes events which capture information on op dispatch, execution
> and retirement, branch prediction, L1 and L2 cache activity,
> TLB activity, etc.
>
> Link: https://bugzilla.kernel.org/attachment.cgi?id=3D305974
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> ---
>  .../pmu-events/arch/x86/amdzen5/branch.json   |  82 ++
>  .../pmu-events/arch/x86/amdzen5/cache.json    | 605 +++++++++++++
>  .../pmu-events/arch/x86/amdzen5/core.json     | 122 +++
>  .../arch/x86/amdzen5/floating-point.json      | 830 ++++++++++++++++++
>  .../pmu-events/arch/x86/amdzen5/memory.json   | 180 ++++
>  .../pmu-events/arch/x86/amdzen5/other.json    | 168 ++++
>  6 files changed, 1987 insertions(+)
>  create mode 100644 tools/perf/pmu-events/arch/x86/amdzen5/branch.json
>  create mode 100644 tools/perf/pmu-events/arch/x86/amdzen5/cache.json
>  create mode 100644 tools/perf/pmu-events/arch/x86/amdzen5/core.json
>  create mode 100644 tools/perf/pmu-events/arch/x86/amdzen5/floating-point=
json
>  create mode 100644 tools/perf/pmu-events/arch/x86/amdzen5/memory.json
>  create mode 100644 tools/perf/pmu-events/arch/x86/amdzen5/other.json
>
> diff --git a/tools/perf/pmu-events/arch/x86/amdzen5/branch.json b/tools/p=
erf/pmu-events/arch/x86/amdzen5/branch.json
> new file mode 100644
> index 000000000000..208c646c59ca
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/x86/amdzen5/branch.json
> @@ -0,0 +1,82 @@
> +[
> +  {
> +    "EventName": "bp_l2_btb_correct",
> +    "EventCode": "0x8b",
> +    "BriefDescription": "L2 branch prediction overrides existing predict=
ion (speculative)."
> +  },
> +  {
> +    "EventName": "bp_dyn_ind_pred",
> +    "EventCode": "0x8e",
> +    "BriefDescription": "Dynamic indirect predictions (branch used the i=
ndirect predictor to make a prediction)."
> +  },
> +  {
> +    "EventName": "bp_de_redirect",
> +    "EventCode": "0x91",
> +    "BriefDescription": "Instruction decoder corrects the predicted targ=
et and resteers the branch predictor."
> +  },
> +  {
> +    "EventName": "ex_ret_brn",
> +    "EventCode": "0xc2",
> +    "BriefDescription": "Retired branch instructions (all types of archi=
tectural control flow changes, including exceptions and interrupts)."
> +  },

So the "bp_" prefix means branch predictor, but here this is an "ex_"
prefix. You've put them both in the topic "branch".

[ ... snip ... ]

> diff --git a/tools/perf/pmu-events/arch/x86/amdzen5/other.json b/tools/pe=
rf/pmu-events/arch/x86/amdzen5/other.json
> new file mode 100644
> index 000000000000..9d49a23622e9
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/x86/amdzen5/other.json
> @@ -0,0 +1,168 @@
> +[
> +  {
> +    "EventName": "bp_redirects.resync",
> +    "EventCode": "0x9f",
> +    "BriefDescription": "Redirects of the branch predictor caused by res=
yncs.",
> +    "UMask": "0x01"
> +  },

Here is a branch predictor "bp_" event but the topic is "other", why
isn't it a topic of branch?

Given the number of events categorized as topic "other" wouldn't it be
better to regenerate the events just using the prefix to set the
topic?

Thanks,
Ian

[ ... snip ... ]