* [PATCH 1/2] perf evsel: Make evsel__env always return a valid env @ 2021-10-04 21:41 Kim Phillips 2021-10-04 21:41 ` [PATCH 2/2] perf tools: Improve IBS error handling Kim Phillips 2021-11-23 8:26 ` [PATCH 1/2] perf evsel: Make evsel__env always return a valid env kajoljain 0 siblings, 2 replies; 13+ messages in thread From: Kim Phillips @ 2021-10-04 21:41 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, kim.phillips Cc: linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Jiri Olsa, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian It's possible to have an evsel and evsel->evlist populated without an evsel->evlist->env, when, e.g., cmd_record is in its error path. Future patches will add support for evsel__open_strerror to be able to customize error messaging based on perf_env__{arch,cpuid}, so let's have evsel__env return &perf_env instead of NULL in that case. Signed-off-by: Kim Phillips <kim.phillips@amd.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Ian Rogers <irogers@google.com> Cc: Ingo Molnar <mingo@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Joao Martins <joao.m.martins@oracle.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Michael Petlan <mpetlan@redhat.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Robert Richter <robert.richter@amd.com> Cc: Stephane Eranian <eranian@google.com> --- tools/perf/util/evsel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index dbfeceb2546c..b915840690d4 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -2857,7 +2857,7 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, struct perf_env *evsel__env(struct evsel *evsel) { - if (evsel && evsel->evlist) + if (evsel && evsel->evlist && evsel->evlist->env) return evsel->evlist->env; return &perf_env; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] perf tools: Improve IBS error handling 2021-10-04 21:41 [PATCH 1/2] perf evsel: Make evsel__env always return a valid env Kim Phillips @ 2021-10-04 21:41 ` Kim Phillips 2021-10-07 17:28 ` Jiri Olsa 2021-11-23 8:26 ` [PATCH 1/2] perf evsel: Make evsel__env always return a valid env kajoljain 1 sibling, 1 reply; 13+ messages in thread From: Kim Phillips @ 2021-10-04 21:41 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, kim.phillips Cc: linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Jiri Olsa, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian This patch improves the error message returned on failed perf_event_open() on AMD when using IBS. Output of executing 'perf record -e ibs_op// true' BEFORE this patch: The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (ibs_op//u). /bin/dmesg | grep -i perf may provide additional information. Output after: AMD IBS can't exclude kernel events. Try running at a higher privilege level. Output of executing 'sudo perf record -e ibs_op// true' BEFORE this patch: Error: The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (ibs_op//). /bin/dmesg | grep -i perf may provide additional information. Output after: Error: AMD IBS may only be available in system-wide/per-cpu mode. Try using -a, or -C and workload affinity Signed-off-by: Kim Phillips <kim.phillips@amd.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Ian Rogers <irogers@google.com> Cc: Ingo Molnar <mingo@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Joao Martins <joao.m.martins@oracle.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Michael Petlan <mpetlan@redhat.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Robert Richter <robert.richter@amd.com> Cc: Stephane Eranian <eranian@google.com> --- 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)) { + 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"); + } + } + break; case ENODATA: return scnprintf(msg, size, "Cannot collect data source with the load latency event alone. " -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-10-04 21:41 ` [PATCH 2/2] perf tools: Improve IBS error handling Kim Phillips @ 2021-10-07 17:28 ` Jiri Olsa 2021-10-07 19:17 ` Kim Phillips 0 siblings, 1 reply; 13+ messages in thread From: Jiri Olsa @ 2021-10-07 17:28 UTC (permalink / raw) To: Kim Phillips Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On Mon, Oct 04, 2021 at 04:41:14PM -0500, Kim Phillips wrote: SNIP > --- > 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 jirka > + 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"); > + } > + } > + > break; > case ENODATA: > return scnprintf(msg, size, "Cannot collect data source with the load latency event alone. " > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-10-07 17:28 ` Jiri Olsa @ 2021-10-07 19:17 ` Kim Phillips 2021-11-18 22:45 ` Kim Phillips 2021-11-23 8:40 ` kajoljain 0 siblings, 2 replies; 13+ messages in thread From: Kim Phillips @ 2021-10-07 19:17 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian, kim.phillips 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 + } } 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@google.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@google.com> [Rebased on commit 9fe8895a27a84 ("perf env: Add perf_env__cpuid, perf_env__{nr_}pmu_mappings")] Signed-off-by: Kim Phillips <kim.phillips@amd.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; -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-10-07 19:17 ` Kim Phillips @ 2021-11-18 22:45 ` Kim Phillips 2021-11-23 8:40 ` kajoljain 1 sibling, 0 replies; 13+ messages in thread From: Kim Phillips @ 2021-11-18 22:45 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On 10/7/21 2:17 PM, 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 > + } > } > > 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? No reply, so guessing not. Arnaldo, can these two patches be applied please? Thanks, Kim ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-10-07 19:17 ` Kim Phillips 2021-11-18 22:45 ` Kim Phillips @ 2021-11-23 8:40 ` kajoljain 2021-11-23 15:25 ` Kim Phillips 1 sibling, 1 reply; 13+ messages in thread From: kajoljain @ 2021-11-23 8:40 UTC (permalink / raw) To: Kim Phillips, Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian 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@google.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@google.com> > [Rebased on commit 9fe8895a27a84 ("perf env: Add perf_env__cpuid, > perf_env__{nr_}pmu_mappings")] > Signed-off-by: Kim Phillips <kim.phillips@amd.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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-11-23 8:40 ` kajoljain @ 2021-11-23 15:25 ` Kim Phillips 2021-11-24 8:00 ` kajoljain 0 siblings, 1 reply; 13+ messages in thread From: Kim Phillips @ 2021-11-23 15:25 UTC (permalink / raw) To: kajoljain, Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On 11/23/21 2:40 AM, kajoljain wrote: > 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."); > + } IIRC, I tried something like that but carrying the struct target *target, int err, char *msg, size_t size parameters made things worse. > 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. The vast majority of decisions made by evsel__open_strerror are going to be common across most arch/uarches. AMD has only these two pesky exceptions to the rule and therefore IMO it's ok to have them inline with the common function, since the decisions are so deeply intertwined. A new amd_evsel_open_strerror_check sounds like it'd duplicate too much of the common function code in order to handle the common error cases. Kim ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-11-23 15:25 ` Kim Phillips @ 2021-11-24 8:00 ` kajoljain 2021-11-29 22:09 ` Kim Phillips 0 siblings, 1 reply; 13+ messages in thread From: kajoljain @ 2021-11-24 8:00 UTC (permalink / raw) To: Kim Phillips, Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On 11/23/21 8:55 PM, Kim Phillips wrote: > On 11/23/21 2:40 AM, kajoljain wrote: >> 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."); >> + } > > IIRC, I tried something like that but carrying the > > > struct target *target, int err, char *msg, size_t size > > parameters made things worse. > >> 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. > > The vast majority of decisions made by evsel__open_strerror are > going to be common across most arch/uarches. AMD has only these > two pesky exceptions to the rule and therefore IMO it's ok > to have them inline with the common function, since the decisions > are so deeply intertwined. A new amd_evsel_open_strerror_check > sounds like it'd duplicate too much of the common function code > in order to handle the common error cases. Hi Kim, Sorry for the confusion, what I meant by adding new function is just to handle these corner error cases and not duplicating whole evsel__open_strerror code. Maybe something like below code, Its just prototype of code to show you the flow, you can refine it and check for any build or indentation issues using checkpatch.pl script. So basically, in common function we can just have 2 calls, first to check if we are in amd system and second to return corresponding error message, rather then adding whole chunk of if's which are specific to amd. diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index ac0127be0459..adefb162ae08 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -2852,9 +2852,40 @@ 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 int error_amd_ibs_brs(struct evsel *evsel, char *msg, size_t size) +{ + if (evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name, "ibs", 3)) { + 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"); + } + + if (((evsel->core.attr.config & 0xff) == 0xc4) && (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK)) { + 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."); + } + } +} + 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; @@ -2950,6 +2981,8 @@ 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)) + return error_amd_ibs_brs(evsel, msg, size); break; case ENODATA: return scnprintf(msg, size, "Cannot collect data source with the load latency event alone. " Thanks, Kajol Jain > > Kim ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-11-24 8:00 ` kajoljain @ 2021-11-29 22:09 ` Kim Phillips 2021-12-08 6:33 ` kajoljain 0 siblings, 1 reply; 13+ messages in thread From: Kim Phillips @ 2021-11-29 22:09 UTC (permalink / raw) To: kajoljain, Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On 11/24/21 2:00 AM, kajoljain wrote: > On 11/23/21 8:55 PM, Kim Phillips wrote: >> On 11/23/21 2:40 AM, kajoljain wrote: >>> 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."); >>> + } >> >> IIRC, I tried something like that but carrying the >> >> >> struct target *target, int err, char *msg, size_t size >> >> parameters made things worse. >> >>> 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. >> >> The vast majority of decisions made by evsel__open_strerror are >> going to be common across most arch/uarches. AMD has only these >> two pesky exceptions to the rule and therefore IMO it's ok >> to have them inline with the common function, since the decisions >> are so deeply intertwined. A new amd_evsel_open_strerror_check >> sounds like it'd duplicate too much of the common function code >> in order to handle the common error cases. > > Hi Kim, > Sorry for the confusion, what I meant by adding new function is just > to handle these corner error cases and not duplicating whole > evsel__open_strerror code. > > Maybe something like below code, Its just prototype of code to show you > the flow, you can refine it and check for any build or indentation > issues using checkpatch.pl script. > > So basically, in common function we can just have 2 calls, first to > check if we are in amd system and second to return corresponding error > message, rather then adding whole chunk of if's which are specific to amd. > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index ac0127be0459..adefb162ae08 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -2852,9 +2852,40 @@ 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 int error_amd_ibs_brs(struct evsel *evsel, char *msg, size_t size) > +{ > + if (evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name, > "ibs", 3)) { > + 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"); > + } > + > + if (((evsel->core.attr.config & 0xff) == 0xc4) && > (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK)) { > + 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."); > + } > + } > +} > + > 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; > > @@ -2950,6 +2981,8 @@ 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)) > + return error_amd_ibs_brs(evsel, msg, size); > break; > case ENODATA: > return scnprintf(msg, size, "Cannot collect data source > with the load latency event alone. " That change will makes AMD machines fail to fall back to the default "The sys_perf_event_open() syscall returned with..." error string in case it's not those AMD IBS and BRS sub-conditions. Is having the AMD error code checking in the main evsel__open_strerror() so bad? Other arches and their PMU implementations may find error conditions that they have in common with AMD's, therefore opening up the code for opposite types of refactoring and reuse than what is being requested here. E.g., I've seen other hardware configurations - not specific to one architecture - that could also use this message: {"AMD IBS"->"%s",pmu_name} may only be available in system-wide/per-cpu mode. Try using -a, or -C and workload affinity"); Thanks, Kim ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-11-29 22:09 ` Kim Phillips @ 2021-12-08 6:33 ` kajoljain 2021-12-08 17:14 ` Kim Phillips 0 siblings, 1 reply; 13+ messages in thread From: kajoljain @ 2021-12-08 6:33 UTC (permalink / raw) To: Kim Phillips, Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On 11/30/21 3:39 AM, Kim Phillips wrote: > On 11/24/21 2:00 AM, kajoljain wrote: >> On 11/23/21 8:55 PM, Kim Phillips wrote: >>> On 11/23/21 2:40 AM, kajoljain wrote: >>>> 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."); >>>> + } >>> >>> IIRC, I tried something like that but carrying the >>> >>> >>> struct target *target, int err, char *msg, size_t size >>> >>> parameters made things worse. >>> >>>> 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. >>> >>> The vast majority of decisions made by evsel__open_strerror are >>> going to be common across most arch/uarches. AMD has only these >>> two pesky exceptions to the rule and therefore IMO it's ok >>> to have them inline with the common function, since the decisions >>> are so deeply intertwined. A new amd_evsel_open_strerror_check >>> sounds like it'd duplicate too much of the common function code >>> in order to handle the common error cases. >> >> Hi Kim, >> Sorry for the confusion, what I meant by adding new function is just >> to handle these corner error cases and not duplicating whole >> evsel__open_strerror code. >> >> Maybe something like below code, Its just prototype of code to show you >> the flow, you can refine it and check for any build or indentation >> issues using checkpatch.pl script. >> >> So basically, in common function we can just have 2 calls, first to >> check if we are in amd system and second to return corresponding error >> message, rather then adding whole chunk of if's which are specific to >> amd. >> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >> index ac0127be0459..adefb162ae08 100644 >> --- a/tools/perf/util/evsel.c >> +++ b/tools/perf/util/evsel.c >> @@ -2852,9 +2852,40 @@ 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 int error_amd_ibs_brs(struct evsel *evsel, char *msg, size_t >> size) >> +{ >> + if (evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name, >> "ibs", 3)) { >> + 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"); >> + } >> + >> + if (((evsel->core.attr.config & 0xff) == 0xc4) && >> (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK)) { >> + 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."); >> + } >> + } >> +} >> + >> 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; >> >> @@ -2950,6 +2981,8 @@ 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)) >> + return error_amd_ibs_brs(evsel, msg, size); >> break; >> case ENODATA: >> return scnprintf(msg, size, "Cannot collect data source >> with the load latency event alone. " > > That change will makes AMD machines fail to fall back to the default > "The sys_perf_event_open() syscall returned with..." error string > in case it's not those AMD IBS and BRS sub-conditions. Yes right, as I mentioned before, the code I pointed was just a prototype to show you the flow, these corner cases can be handled on top of it. > > Is having the AMD error code checking in the main evsel__open_strerror() > so bad? Other arches and their PMU implementations may find error > conditions that they have in common with AMD's, therefore > opening up the code for opposite types of refactoring and > reuse than what is being requested here. E.g., I've seen > other hardware configurations - not specific to one architecture - > that could also use this message: > From my understanding, adding too many checks in common function for a specific arch is not a good practice. Since you already adding multiple functions to get information like ,if current system is amd/ibs/brs. Can't we rather just add a single function and handled all these checks there? That's just my thoughts, if maintainers are ok with it, then its fine for me too. Thanks, Kajol Jain > {"AMD IBS"->"%s",pmu_name} may only be available in system-wide/per-cpu > mode. Try using -a, or -C and workload affinity"); > > Thanks, > > Kim ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] perf tools: Improve IBS error handling 2021-12-08 6:33 ` kajoljain @ 2021-12-08 17:14 ` Kim Phillips 0 siblings, 0 replies; 13+ messages in thread From: Kim Phillips @ 2021-12-08 17:14 UTC (permalink / raw) To: kajoljain, Jiri Olsa Cc: Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On 12/8/21 12:33 AM, kajoljain wrote: > On 11/30/21 3:39 AM, Kim Phillips wrote: >> On 11/24/21 2:00 AM, kajoljain wrote: >>> On 11/23/21 8:55 PM, Kim Phillips wrote: >>>> On 11/23/21 2:40 AM, kajoljain wrote: >>>>> 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."); >>>>> + } >>>> >>>> IIRC, I tried something like that but carrying the >>>> >>>> >>>> struct target *target, int err, char *msg, size_t size >>>> >>>> parameters made things worse. >>>> >>>>> 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. >>>> >>>> The vast majority of decisions made by evsel__open_strerror are >>>> going to be common across most arch/uarches. AMD has only these >>>> two pesky exceptions to the rule and therefore IMO it's ok >>>> to have them inline with the common function, since the decisions >>>> are so deeply intertwined. A new amd_evsel_open_strerror_check >>>> sounds like it'd duplicate too much of the common function code >>>> in order to handle the common error cases. >>> >>> Hi Kim, >>> Sorry for the confusion, what I meant by adding new function is just >>> to handle these corner error cases and not duplicating whole >>> evsel__open_strerror code. >>> >>> Maybe something like below code, Its just prototype of code to show you >>> the flow, you can refine it and check for any build or indentation >>> issues using checkpatch.pl script. >>> >>> So basically, in common function we can just have 2 calls, first to >>> check if we are in amd system and second to return corresponding error >>> message, rather then adding whole chunk of if's which are specific to >>> amd. >>> >>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >>> index ac0127be0459..adefb162ae08 100644 >>> --- a/tools/perf/util/evsel.c >>> +++ b/tools/perf/util/evsel.c >>> @@ -2852,9 +2852,40 @@ 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 int error_amd_ibs_brs(struct evsel *evsel, char *msg, size_t >>> size) >>> +{ >>> + if (evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name, >>> "ibs", 3)) { >>> + 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"); >>> + } >>> + >>> + if (((evsel->core.attr.config & 0xff) == 0xc4) && >>> (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK)) { >>> + 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."); >>> + } >>> + } >>> +} >>> + >>> 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; >>> >>> @@ -2950,6 +2981,8 @@ 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)) >>> + return error_amd_ibs_brs(evsel, msg, size); >>> break; >>> case ENODATA: >>> return scnprintf(msg, size, "Cannot collect data source >>> with the load latency event alone. " >> >> That change will makes AMD machines fail to fall back to the default >> "The sys_perf_event_open() syscall returned with..." error string >> in case it's not those AMD IBS and BRS sub-conditions. > > Yes right, as I mentioned before, the code I pointed was just a > prototype to show you the flow, these corner cases can be handled on top > of it. Right but these corner cases disrupt the existing flow: adding int ret; ret = foo(); if (ret) goto report_generic_einval doesn't go with the flow. >> Is having the AMD error code checking in the main evsel__open_strerror() >> so bad? Other arches and their PMU implementations may find error >> conditions that they have in common with AMD's, therefore >> opening up the code for opposite types of refactoring and >> reuse than what is being requested here. E.g., I've seen >> other hardware configurations - not specific to one architecture - >> that could also use this message: >> > > From my understanding, adding too many checks in common function > for a specific arch is not a good practice. Since you already adding My point above is that other arches can come in and adopt the same error conditions and text. > multiple functions to get information like ,if current system is > amd/ibs/brs. Can't we rather just add a single function and handled all > these checks there? That will remove the code from the common path. Code that's possible (and in some cases likely) that will be adopted by other arches. Thanks, Kim ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] perf evsel: Make evsel__env always return a valid env 2021-10-04 21:41 [PATCH 1/2] perf evsel: Make evsel__env always return a valid env Kim Phillips 2021-10-04 21:41 ` [PATCH 2/2] perf tools: Improve IBS error handling Kim Phillips @ 2021-11-23 8:26 ` kajoljain 2022-03-22 21:36 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 13+ messages in thread From: kajoljain @ 2021-11-23 8:26 UTC (permalink / raw) To: Kim Phillips, Arnaldo Carvalho de Melo Cc: linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Jiri Olsa, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian On 10/5/21 3:11 AM, Kim Phillips wrote: > It's possible to have an evsel and evsel->evlist populated without > an evsel->evlist->env, when, e.g., cmd_record is in its error path. > > Future patches will add support for evsel__open_strerror to be able > to customize error messaging based on perf_env__{arch,cpuid}, so > let's have evsel__env return &perf_env instead of NULL in that case. > Patch looks good to me. Reviewed-by: Kajol Jain<kjain@linux.ibm.com> Thanks, Kajol Jain > Signed-off-by: Kim Phillips <kim.phillips@amd.com> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Ian Rogers <irogers@google.com> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Joao Martins <joao.m.martins@oracle.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Michael Petlan <mpetlan@redhat.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Robert Richter <robert.richter@amd.com> > Cc: Stephane Eranian <eranian@google.com> > --- > tools/perf/util/evsel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index dbfeceb2546c..b915840690d4 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -2857,7 +2857,7 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, > > struct perf_env *evsel__env(struct evsel *evsel) > { > - if (evsel && evsel->evlist) > + if (evsel && evsel->evlist && evsel->evlist->env) > return evsel->evlist->env; > return &perf_env; > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] perf evsel: Make evsel__env always return a valid env 2021-11-23 8:26 ` [PATCH 1/2] perf evsel: Make evsel__env always return a valid env kajoljain @ 2022-03-22 21:36 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 13+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-03-22 21:36 UTC (permalink / raw) To: kajoljain Cc: Kim Phillips, Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Alexander Shishkin, Boris Ostrovsky, Ian Rogers, Ingo Molnar, Jiri Olsa, Joao Martins, Konrad Rzeszutek Wilk, Mark Rutland, Michael Petlan, Namhyung Kim, Peter Zijlstra, Robert Richter, Stephane Eranian Em Tue, Nov 23, 2021 at 01:56:38PM +0530, kajoljain escreveu: > > > On 10/5/21 3:11 AM, Kim Phillips wrote: > > It's possible to have an evsel and evsel->evlist populated without > > an evsel->evlist->env, when, e.g., cmd_record is in its error path. > > > > Future patches will add support for evsel__open_strerror to be able > > to customize error messaging based on perf_env__{arch,cpuid}, so > > let's have evsel__env return &perf_env instead of NULL in that case. > > > > Patch looks good to me. > > Reviewed-by: Kajol Jain<kjain@linux.ibm.com> Collecting this one now, found it thru the cracks. - Arnaldo > Thanks, > Kajol Jain > > > Signed-off-by: Kim Phillips <kim.phillips@amd.com> > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Cc: Ian Rogers <irogers@google.com> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Jiri Olsa <jolsa@redhat.com> > > Cc: Joao Martins <joao.m.martins@oracle.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Michael Petlan <mpetlan@redhat.com> > > Cc: Namhyung Kim <namhyung@kernel.org> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Robert Richter <robert.richter@amd.com> > > Cc: Stephane Eranian <eranian@google.com> > > --- > > tools/perf/util/evsel.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > > index dbfeceb2546c..b915840690d4 100644 > > --- a/tools/perf/util/evsel.c > > +++ b/tools/perf/util/evsel.c > > @@ -2857,7 +2857,7 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, > > > > struct perf_env *evsel__env(struct evsel *evsel) > > { > > - if (evsel && evsel->evlist) > > + if (evsel && evsel->evlist && evsel->evlist->env) > > return evsel->evlist->env; > > return &perf_env; > > } > > -- - Arnaldo ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-03-22 21:36 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-04 21:41 [PATCH 1/2] perf evsel: Make evsel__env always return a valid env Kim Phillips 2021-10-04 21:41 ` [PATCH 2/2] perf tools: Improve IBS error handling Kim Phillips 2021-10-07 17:28 ` Jiri Olsa 2021-10-07 19:17 ` Kim Phillips 2021-11-18 22:45 ` Kim Phillips 2021-11-23 8:40 ` kajoljain 2021-11-23 15:25 ` Kim Phillips 2021-11-24 8:00 ` kajoljain 2021-11-29 22:09 ` Kim Phillips 2021-12-08 6:33 ` kajoljain 2021-12-08 17:14 ` Kim Phillips 2021-11-23 8:26 ` [PATCH 1/2] perf evsel: Make evsel__env always return a valid env kajoljain 2022-03-22 21:36 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).