| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fdcfec83-01c6-5e25-5b99-dac05287fdae@linux.ibm.com>
Date: Tue, 23 Nov 2021 14:10:53 +0530
From: kajoljain <kjain@...ux.ibm.com>
To: Kim Phillips <kim.phillips@....com>, Jiri Olsa <jolsa@...hat.com>
Cc: Arnaldo Carvalho de Melo <acme@...hat.com>,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
Ian Rogers <irogers@...gle.com>,
Ingo Molnar <mingo@...hat.com>,
Joao Martins <joao.m.martins@...cle.com>,
Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
Mark Rutland <mark.rutland@....com>,
Michael Petlan <mpetlan@...hat.com>,
Namhyung Kim <namhyung@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Robert Richter <robert.richter@....com>,
Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 2/2] perf tools: Improve IBS error handling
On 10/8/21 12:47 AM, Kim Phillips wrote:
> On 10/7/21 12:28 PM, Jiri Olsa wrote:
>> On Mon, Oct 04, 2021 at 04:41:14PM -0500, Kim Phillips wrote:
>>> ---
>>> tools/perf/util/evsel.c | 24 ++++++++++++++++++++++++
>>> 1 file changed, 24 insertions(+)
>>>
>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>> index b915840690d4..f8a9cbd99314 100644
>>> --- a/tools/perf/util/evsel.c
>>> +++ b/tools/perf/util/evsel.c
>>> @@ -2743,9 +2743,22 @@ static bool find_process(const char *name)
>>> return ret ? false : true;
>>> }
>>> +static bool is_amd(const char *arch, const char *cpuid)
>>> +{
>>> + return arch && !strcmp("x86", arch) && cpuid && strstarts(cpuid,
>>> "AuthenticAMD");
>>> +}
>>> +
>>> +static bool is_amd_ibs(struct evsel *evsel)
>>> +{
>>> + return evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name,
>>> "ibs", 3);
>>> +}
>>> +
>>> int evsel__open_strerror(struct evsel *evsel, struct target *target,
>>> int err, char *msg, size_t size)
>>> {
>>> + struct perf_env *env = evsel__env(evsel);
>>> + const char *arch = perf_env__arch(env);
>>> + const char *cpuid = perf_env__cpuid(env);
>>> char sbuf[STRERR_BUFSIZE];
>>> int printed = 0, enforced = 0;
>>> @@ -2841,6 +2854,17 @@ int evsel__open_strerror(struct evsel
>>> *evsel, struct target *target,
>>> return scnprintf(msg, size, "wrong clockid (%d).",
>>> clockid);
>>> if (perf_missing_features.aux_output)
>>> return scnprintf(msg, size, "The 'aux_output' feature
>>> is not supported, update the kernel.");
>>> + if (is_amd(arch, cpuid)) {
>>> + if (is_amd_ibs(evsel)) {
>>
>> would single 'is_amd_ibs' call be better? checking on both amd and ibs
>
> Good suggestion. If you look at the later patch in the
> BRS series, I have rewritten it to add the new
> AMD PMU like so:
>
> if (is_amd()) {
> if (is_amd_ibs()) {
> if (evsel->this)
> return
> if (evsel->that)
> return
> }
> + if (is_amd_brs()) {
> + if (evsel->this)
> + return
> + if (evsel->that)
> + return
> + }
> }
Hi Kim,
From my point of view, it won't be a good idea of adding so many
checks in common function definition itself.
Can you just create a check to see if its amd machine and then add a
function call which will handle all four conditions together?
which is basically for:
+ if (is_amd(arch, cpuid)) {
+ if (is_amd_ibs(evsel)) {
+ if (evsel->core.attr.exclude_kernel)
+ return scnprintf(msg, size,
+ "AMD IBS can't exclude kernel events. Try running at a higher
privilege level.");
+ if (!evsel->core.system_wide)
+ return scnprintf(msg, size,
+ "AMD IBS may only be available in system-wide/per-cpu mode. Try using
-a, or -C and workload affinity");
+ }
and this:
+ if (is_amd_brs(evsel)) {
+ if (evsel->core.attr.freq)
+ return scnprintf(msg, size,
+ "AMD Branch Sampling does not support frequency mode sampling, must
pass a fixed sampling period via -c option or
cpu/branch-brs,period=xxxx/.");
+ /* another reason is that the period is too small */
+ return scnprintf(msg, size,
+ "AMD Branch Sampling does not support sampling period smaller than
what is reported in /sys/devices/cpu/caps/branches.");
+ }
So, incase we are in amd machine, common function evsel__open_strerror
will call function may be something like amd_evesel_open_strerror_check
which will look for both ibs and brs conditions and return corresponding
error statement.
Thanks,
Kajol Jain
>
> Below is the full proposed replacement patch for patch 12
> of 13 of the BRS series.
>
> Another option is to have the is_amd_{ibs,brs} functions
> call is_amd() themselves, so the if (evsel->) code could be
> unindented by one tab, would that be better?
>
> Thanks,
>
> Kim
>
> From a4cbab762719b30bddec2e278cf8b8eb82e83865 Mon Sep 17 00:00:00 2001
> From: Stephane Eranian <eranian@...gle.com>
> Date: Thu, 9 Sep 2021 00:56:59 -0700
> Subject: [PATCH] perf tools: Improve error handling of AMD Branch Sampling
>
> This patch improves the error message printed by perf when
> perf_event_open() fails on AMD Zen3 when using the branch sampling
> feature. In the case of EINVAL, there are two main reasons: frequency
> mode or period is smaller than the depth of the branch sampling
> buffer (16). The patch checks the parameters of the call and tries
> to print a relevant message to explain the error:
>
> $ perf record -b -e cpu/branch-brs/ -c 10 ls
> Error:
> AMD Branch Sampling does not support sampling period smaller than what
> is reported in /sys/devices/cpu/caps/branches.
>
> $ perf record -b -e cpu/branch-brs/ ls
> Error:
> AMD Branch Sampling does not support frequency mode sampling, must pass
> a fixed sampling period via -c option or cpu/branch-brs,period=xxxx/.
>
> Signed-off-by: Stephane Eranian <eranian@...gle.com>
> [Rebased on commit 9fe8895a27a84 ("perf env: Add perf_env__cpuid,
> perf_env__{nr_}pmu_mappings")]
> Signed-off-by: Kim Phillips <kim.phillips@....com>
> ---
> tools/perf/util/evsel.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index f8a9cbd99314..e1f5eff07355 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2753,6 +2753,12 @@ static bool is_amd_ibs(struct evsel *evsel)
> return evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name,
> "ibs", 3);
> }
>
> +static bool is_amd_brs(struct evsel *evsel)
> +{
> + return ((evsel->core.attr.config & 0xff) == 0xc4) &&
> + (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK);
> +}
> +
> int evsel__open_strerror(struct evsel *evsel, struct target *target,
> int err, char *msg, size_t size)
> {
> @@ -2863,6 +2869,14 @@ int evsel__open_strerror(struct evsel *evsel,
> struct target *target,
> return scnprintf(msg, size,
> "AMD IBS may only be available in system-wide/per-cpu mode. Try
> using -a, or -C and workload affinity");
> }
> + if (is_amd_brs(evsel)) {
> + if (evsel->core.attr.freq)
> + return scnprintf(msg, size,
> + "AMD Branch Sampling does not support frequency mode sampling, must
> pass a fixed sampling period via -c option or
> cpu/branch-brs,period=xxxx/.");
> + /* another reason is that the period is too small */
> + return scnprintf(msg, size,
> + "AMD Branch Sampling does not support sampling period smaller than
> what is reported in /sys/devices/cpu/caps/branches.");
> + }
> }
>
> break;
Powered by blists - more mailing lists