* [PATCH] perf/amd: Prevent grouping of IBS events @ 2023-06-20 9:16 Ravi Bangoria 2023-06-20 16:44 ` Ian Rogers 2023-07-10 8:37 ` [tip: perf/core] " tip-bot2 for Ravi Bangoria 0 siblings, 2 replies; 14+ messages in thread From: Ravi Bangoria @ 2023-06-20 9:16 UTC (permalink / raw) To: peterz Cc: ravi.bangoria, acme, irogers, jolsa, namhyung, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla IBS PMUs can have only one event active at any point in time. Restrict grouping of multiple IBS events. Reported-by: Sandipan Das <sandipan.das@amd.com> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> --- arch/x86/events/amd/ibs.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c index 371014802191..74e664266753 100644 --- a/arch/x86/events/amd/ibs.c +++ b/arch/x86/events/amd/ibs.c @@ -247,11 +247,33 @@ int forward_event_to_ibs(struct perf_event *event) return -ENOENT; } +/* + * Grouping of IBS events is not possible since IBS can have only + * one event active at any point in time. + */ +static int validate_group(struct perf_event *event) +{ + struct perf_event *sibling; + + if (event->group_leader == event) + return 0; + + if (event->group_leader->pmu == event->pmu) + return -EINVAL; + + for_each_sibling_event(sibling, event->group_leader) { + if (sibling->pmu == event->pmu) + return -EINVAL; + } + return 0; +} + static int perf_ibs_init(struct perf_event *event) { struct hw_perf_event *hwc = &event->hw; struct perf_ibs *perf_ibs; u64 max_cnt, config; + int ret; perf_ibs = get_ibs_pmu(event->attr.type); if (!perf_ibs) @@ -265,6 +287,10 @@ static int perf_ibs_init(struct perf_event *event) if (config & ~perf_ibs->config_mask) return -EINVAL; + ret = validate_group(event); + if (ret) + return ret; + if (hwc->sample_period) { if (config & perf_ibs->cnt_mask) /* raw max_cnt may not be set */ -- 2.41.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-20 9:16 [PATCH] perf/amd: Prevent grouping of IBS events Ravi Bangoria @ 2023-06-20 16:44 ` Ian Rogers 2023-06-21 3:27 ` Ravi Bangoria 2023-07-10 8:37 ` [tip: perf/core] " tip-bot2 for Ravi Bangoria 1 sibling, 1 reply; 14+ messages in thread From: Ian Rogers @ 2023-06-20 16:44 UTC (permalink / raw) To: Ravi Bangoria Cc: peterz, acme, jolsa, namhyung, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > IBS PMUs can have only one event active at any point in time. Restrict > grouping of multiple IBS events. Thanks Ravi, can you provide an example/test for this? Should this be a weak group issue? Thanks, Ian > Reported-by: Sandipan Das <sandipan.das@amd.com> > Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> > --- > arch/x86/events/amd/ibs.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c > index 371014802191..74e664266753 100644 > --- a/arch/x86/events/amd/ibs.c > +++ b/arch/x86/events/amd/ibs.c > @@ -247,11 +247,33 @@ int forward_event_to_ibs(struct perf_event *event) > return -ENOENT; > } > > +/* > + * Grouping of IBS events is not possible since IBS can have only > + * one event active at any point in time. > + */ > +static int validate_group(struct perf_event *event) > +{ > + struct perf_event *sibling; > + > + if (event->group_leader == event) > + return 0; > + > + if (event->group_leader->pmu == event->pmu) > + return -EINVAL; > + > + for_each_sibling_event(sibling, event->group_leader) { > + if (sibling->pmu == event->pmu) > + return -EINVAL; > + } > + return 0; > +} > + > static int perf_ibs_init(struct perf_event *event) > { > struct hw_perf_event *hwc = &event->hw; > struct perf_ibs *perf_ibs; > u64 max_cnt, config; > + int ret; > > perf_ibs = get_ibs_pmu(event->attr.type); > if (!perf_ibs) > @@ -265,6 +287,10 @@ static int perf_ibs_init(struct perf_event *event) > if (config & ~perf_ibs->config_mask) > return -EINVAL; > > + ret = validate_group(event); > + if (ret) > + return ret; > + > if (hwc->sample_period) { > if (config & perf_ibs->cnt_mask) > /* raw max_cnt may not be set */ > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-20 16:44 ` Ian Rogers @ 2023-06-21 3:27 ` Ravi Bangoria 2023-06-21 6:23 ` [PATCH] perf evsel amd: Fix IBS error message Ravi Bangoria ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Ravi Bangoria @ 2023-06-21 3:27 UTC (permalink / raw) To: Ian Rogers Cc: peterz, acme, jolsa, namhyung, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla, Ravi Bangoria Hi Ian, On 20-Jun-23 10:14 PM, Ian Rogers wrote: > On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote: >> >> IBS PMUs can have only one event active at any point in time. Restrict >> grouping of multiple IBS events. > > Thanks Ravi, > > can you provide an example/test for this? Should this be a weak group issue? Before: $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 ^C[ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ] After: $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 Error: AMD IBS may only be available in system-wide/per-cpu mode. Try using -a, or -C and workload affinity The error message is stale and misleading. I have a patch to fix it. I'll post it separately. Thanks, Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] perf evsel amd: Fix IBS error message 2023-06-21 3:27 ` Ravi Bangoria @ 2023-06-21 6:23 ` Ravi Bangoria 2023-06-21 22:33 ` Namhyung Kim 2023-06-21 22:50 ` [PATCH] perf/amd: Prevent grouping of IBS events Namhyung Kim 2023-06-22 5:09 ` Ian Rogers 2 siblings, 1 reply; 14+ messages in thread From: Ravi Bangoria @ 2023-06-21 6:23 UTC (permalink / raw) To: acme Cc: ravi.bangoria, peterz, irogers, jolsa, namhyung, adrian.hunter, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla AMD IBS can do per-process profiling[1] and is no longer restricted to per-cpu or systemwide only. Remove stale error message. Before: $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 Error: AMD IBS may only be available in system-wide/per-cpu mode. Try using -a, or -C and workload affinity After: $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 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. [1] https://git.kernel.org/torvalds/c/30093056f7b2 Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> --- tools/perf/util/evsel.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index 356c07f03be6..65b0b70830f0 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, return scnprintf(msg, size, "Invalid event (%s) in per-thread mode, enable system wide with '-a'.", evsel__name(evsel)); - if (is_amd(arch, cpuid)) { - if (is_amd_ibs(evsel)) { - if (evsel->core.attr.exclude_kernel) - return scnprintf(msg, size, + if (is_amd(arch, cpuid) && 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"); } } -- 2.41.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] perf evsel amd: Fix IBS error message 2023-06-21 6:23 ` [PATCH] perf evsel amd: Fix IBS error message Ravi Bangoria @ 2023-06-21 22:33 ` Namhyung Kim 2023-06-22 5:27 ` Ravi Bangoria 0 siblings, 1 reply; 14+ messages in thread From: Namhyung Kim @ 2023-06-21 22:33 UTC (permalink / raw) To: Ravi Bangoria Cc: acme, peterz, irogers, jolsa, adrian.hunter, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla Hi Ravi, On Tue, Jun 20, 2023 at 11:24 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > AMD IBS can do per-process profiling[1] and is no longer restricted to > per-cpu or systemwide only. Remove stale error message. > > Before: > $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > Error: > AMD IBS may only be available in system-wide/per-cpu mode. > Try using -a, or -C and workload affinity It was strange that the -C option was given already. > > After: > $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > 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. It can run newer perf tools on an old kernel but the old error message seems to be invalid anyway. So I'm ok with removing it. > > [1] https://git.kernel.org/torvalds/c/30093056f7b2 > > Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> > --- > tools/perf/util/evsel.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 356c07f03be6..65b0b70830f0 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, > return scnprintf(msg, size, > "Invalid event (%s) in per-thread mode, enable system wide with '-a'.", > evsel__name(evsel)); > - if (is_amd(arch, cpuid)) { > - if (is_amd_ibs(evsel)) { > - if (evsel->core.attr.exclude_kernel) > - return scnprintf(msg, size, > + if (is_amd(arch, cpuid) && 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."); I'm not sure if it's enough. The IBS PMUs have CAP_NO_EXCLUDE then it can't exclude user events too, right? Thanks, Namhyung > - 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"); > } > } > > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf evsel amd: Fix IBS error message 2023-06-21 22:33 ` Namhyung Kim @ 2023-06-22 5:27 ` Ravi Bangoria 2023-06-22 5:38 ` Namhyung Kim 0 siblings, 1 reply; 14+ messages in thread From: Ravi Bangoria @ 2023-06-22 5:27 UTC (permalink / raw) To: Namhyung Kim Cc: acme, peterz, irogers, jolsa, adrian.hunter, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla, Ravi Bangoria Hi Namhyung, On 22-Jun-23 4:03 AM, Namhyung Kim wrote: > Hi Ravi, > > On Tue, Jun 20, 2023 at 11:24 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: >> >> AMD IBS can do per-process profiling[1] and is no longer restricted to >> per-cpu or systemwide only. Remove stale error message. >> >> Before: >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 >> Error: >> AMD IBS may only be available in system-wide/per-cpu mode. >> Try using -a, or -C and workload affinity > > It was strange that the -C option was given already. Hmm. evsel->core.system_wide is bit confusing for me. A Comment on it's definition says "irrespective of user requested CPUs or threads": /* * system_wide is for events that need to be on every CPU, irrespective * of user requested CPUs or threads. Map propagation will set cpus to * this event's own_cpus, whereby they will contribute to evlist * all_cpus. */ bool system_wide; So, I guess evsel->core.system_wide doesn't depend on -a / -C. > >> >> After: >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 >> 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. > > It can run newer perf tools on an old kernel but the old error > message seems to be invalid anyway. So I'm ok with removing it. Right. >> >> [1] https://git.kernel.org/torvalds/c/30093056f7b2 >> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> >> --- >> tools/perf/util/evsel.c | 10 +++------- >> 1 file changed, 3 insertions(+), 7 deletions(-) >> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >> index 356c07f03be6..65b0b70830f0 100644 >> --- a/tools/perf/util/evsel.c >> +++ b/tools/perf/util/evsel.c >> @@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, >> return scnprintf(msg, size, >> "Invalid event (%s) in per-thread mode, enable system wide with '-a'.", >> evsel__name(evsel)); >> - if (is_amd(arch, cpuid)) { >> - if (is_amd_ibs(evsel)) { >> - if (evsel->core.attr.exclude_kernel) >> - return scnprintf(msg, size, >> + if (is_amd(arch, cpuid) && 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."); > > I'm not sure if it's enough. The IBS PMUs have CAP_NO_EXCLUDE then > it can't exclude user events too, right? That's correct. Let me try to fix this whole `if (is_amd())` part properly. Thanks for your feedback, Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf evsel amd: Fix IBS error message 2023-06-22 5:27 ` Ravi Bangoria @ 2023-06-22 5:38 ` Namhyung Kim 0 siblings, 0 replies; 14+ messages in thread From: Namhyung Kim @ 2023-06-22 5:38 UTC (permalink / raw) To: Ravi Bangoria Cc: acme, peterz, irogers, jolsa, adrian.hunter, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla On Wed, Jun 21, 2023 at 10:28 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > Hi Namhyung, > > On 22-Jun-23 4:03 AM, Namhyung Kim wrote: > > Hi Ravi, > > > > On Tue, Jun 20, 2023 at 11:24 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > >> > >> AMD IBS can do per-process profiling[1] and is no longer restricted to > >> per-cpu or systemwide only. Remove stale error message. > >> > >> Before: > >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > >> Error: > >> AMD IBS may only be available in system-wide/per-cpu mode. > >> Try using -a, or -C and workload affinity > > > > It was strange that the -C option was given already. > > Hmm. evsel->core.system_wide is bit confusing for me. A Comment on it's > definition says "irrespective of user requested CPUs or threads": > > /* > * system_wide is for events that need to be on every CPU, irrespective > * of user requested CPUs or threads. Map propagation will set cpus to > * this event's own_cpus, whereby they will contribute to evlist > * all_cpus. > */ > bool system_wide; > > So, I guess evsel->core.system_wide doesn't depend on -a / -C. Right, you shouldn't check this flag. IIRC It's used by Intel PT for dummy events to get sideband events from every CPU regardless of the target. The proper check would be target__has_cpu() and it seems the evsel__open_strerror() already checks that. Thanks, Namhyung > > > > >> > >> After: > >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > >> 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. > > > > It can run newer perf tools on an old kernel but the old error > > message seems to be invalid anyway. So I'm ok with removing it. > > Right. > > >> > >> [1] https://git.kernel.org/torvalds/c/30093056f7b2 > >> > >> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> > >> --- > >> tools/perf/util/evsel.c | 10 +++------- > >> 1 file changed, 3 insertions(+), 7 deletions(-) > >> > >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > >> index 356c07f03be6..65b0b70830f0 100644 > >> --- a/tools/perf/util/evsel.c > >> +++ b/tools/perf/util/evsel.c > >> @@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, > >> return scnprintf(msg, size, > >> "Invalid event (%s) in per-thread mode, enable system wide with '-a'.", > >> evsel__name(evsel)); > >> - if (is_amd(arch, cpuid)) { > >> - if (is_amd_ibs(evsel)) { > >> - if (evsel->core.attr.exclude_kernel) > >> - return scnprintf(msg, size, > >> + if (is_amd(arch, cpuid) && 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."); > > > > I'm not sure if it's enough. The IBS PMUs have CAP_NO_EXCLUDE then > > it can't exclude user events too, right? > > That's correct. Let me try to fix this whole `if (is_amd())` part properly. > > Thanks for your feedback, > Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-21 3:27 ` Ravi Bangoria 2023-06-21 6:23 ` [PATCH] perf evsel amd: Fix IBS error message Ravi Bangoria @ 2023-06-21 22:50 ` Namhyung Kim 2023-06-22 3:30 ` Ravi Bangoria 2023-06-22 5:09 ` Ian Rogers 2 siblings, 1 reply; 14+ messages in thread From: Namhyung Kim @ 2023-06-21 22:50 UTC (permalink / raw) To: Ravi Bangoria Cc: Ian Rogers, peterz, acme, jolsa, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla On Tue, Jun 20, 2023 at 8:28 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > Hi Ian, > > On 20-Jun-23 10:14 PM, Ian Rogers wrote: > > On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > >> > >> IBS PMUs can have only one event active at any point in time. Restrict > >> grouping of multiple IBS events. > > > > Thanks Ravi, > > > > can you provide an example/test for this? Should this be a weak group issue? > > Before: > $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > ^C[ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ] > > After: > $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > Error: > AMD IBS may only be available in system-wide/per-cpu mode. > Try using -a, or -C and workload affinity > > The error message is stale and misleading. I have a patch to fix it. > I'll post it separately. I'm just curious if ibs_fetch and ibs_op can be grouped together. Thanks, Namhyung ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-21 22:50 ` [PATCH] perf/amd: Prevent grouping of IBS events Namhyung Kim @ 2023-06-22 3:30 ` Ravi Bangoria 0 siblings, 0 replies; 14+ messages in thread From: Ravi Bangoria @ 2023-06-22 3:30 UTC (permalink / raw) To: Namhyung Kim Cc: Ian Rogers, peterz, acme, jolsa, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla, Ravi Bangoria Hi Namhyung, > I'm just curious if ibs_fetch and ibs_op can be grouped together. No. Both are independent hardware pmus and thus can not be grouped together. Thanks, Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-21 3:27 ` Ravi Bangoria 2023-06-21 6:23 ` [PATCH] perf evsel amd: Fix IBS error message Ravi Bangoria 2023-06-21 22:50 ` [PATCH] perf/amd: Prevent grouping of IBS events Namhyung Kim @ 2023-06-22 5:09 ` Ian Rogers 2023-06-22 5:39 ` Ravi Bangoria 2 siblings, 1 reply; 14+ messages in thread From: Ian Rogers @ 2023-06-22 5:09 UTC (permalink / raw) To: Ravi Bangoria Cc: peterz, acme, jolsa, namhyung, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla On Tue, Jun 20, 2023 at 8:27 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > Hi Ian, > > On 20-Jun-23 10:14 PM, Ian Rogers wrote: > > On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > >> > >> IBS PMUs can have only one event active at any point in time. Restrict > >> grouping of multiple IBS events. > > > > Thanks Ravi, > > > > can you provide an example/test for this? Should this be a weak group issue? > > Before: > $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > ^C[ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ] > > After: > $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > Error: > AMD IBS may only be available in system-wide/per-cpu mode. > Try using -a, or -C and workload affinity > > The error message is stale and misleading. I have a patch to fix it. > I'll post it separately. Thanks Ravi, so this is a workaround for a PMU driver bug where the perf_event_open should have failed for the sibling event? The behavior is somewhat reminiscent of arch_evsel__must_be_in_group: https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evsel.c?h=perf-tools-next#n41 Normally software events would be valid in the group, should the code ignore these? Thanks, Ian > Thanks, > Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-22 5:09 ` Ian Rogers @ 2023-06-22 5:39 ` Ravi Bangoria 2023-06-22 5:44 ` Ian Rogers 0 siblings, 1 reply; 14+ messages in thread From: Ravi Bangoria @ 2023-06-22 5:39 UTC (permalink / raw) To: Ian Rogers Cc: peterz, acme, jolsa, namhyung, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla, Ravi Bangoria Hi Ian, >> Before: >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 >> ^C[ perf record: Woken up 1 times to write data ] >> [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ] >> >> After: >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 >> Error: >> AMD IBS may only be available in system-wide/per-cpu mode. >> Try using -a, or -C and workload affinity >> >> The error message is stale and misleading. I have a patch to fix it. >> I'll post it separately. > > Thanks Ravi, so this is a workaround for a PMU driver bug where the > perf_event_open should have failed for the sibling event? This is not a workaround. This kernel patch fixes PMU driver bug. With the patch, perf_event_open() will fail for sibling IBS event if either group leader or any other sibling is of the same IBS pmu. Or did I misread your comment? > > The behavior is somewhat reminiscent of arch_evsel__must_be_in_group: > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evsel.c?h=perf-tools-next#n41 > > Normally software events would be valid in the group, should the code > ignore these? Grouping of SW and IBS event will continue to work after this patch. Thanks, Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-22 5:39 ` Ravi Bangoria @ 2023-06-22 5:44 ` Ian Rogers 2023-06-22 5:49 ` Ravi Bangoria 0 siblings, 1 reply; 14+ messages in thread From: Ian Rogers @ 2023-06-22 5:44 UTC (permalink / raw) To: Ravi Bangoria Cc: peterz, acme, jolsa, namhyung, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla On Wed, Jun 21, 2023 at 10:39 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > Hi Ian, > > >> Before: > >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > >> ^C[ perf record: Woken up 1 times to write data ] > >> [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ] > >> > >> After: > >> $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0 > >> Error: > >> AMD IBS may only be available in system-wide/per-cpu mode. > >> Try using -a, or -C and workload affinity > >> > >> The error message is stale and misleading. I have a patch to fix it. > >> I'll post it separately. > > > > Thanks Ravi, so this is a workaround for a PMU driver bug where the > > perf_event_open should have failed for the sibling event? > > This is not a workaround. This kernel patch fixes PMU driver bug. With > the patch, perf_event_open() will fail for sibling IBS event if either > group leader or any other sibling is of the same IBS pmu. Or did I > misread your comment? > > > > > The behavior is somewhat reminiscent of arch_evsel__must_be_in_group: > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evsel.c?h=perf-tools-next#n41 > > > > Normally software events would be valid in the group, should the code > > ignore these? > > Grouping of SW and IBS event will continue to work after this patch. Sorry Ravi, I've got my head in the clouds. I was reading this as a tools patch :-) Thanks, Ian > Thanks, > Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] perf/amd: Prevent grouping of IBS events 2023-06-22 5:44 ` Ian Rogers @ 2023-06-22 5:49 ` Ravi Bangoria 0 siblings, 0 replies; 14+ messages in thread From: Ravi Bangoria @ 2023-06-22 5:49 UTC (permalink / raw) To: Ian Rogers Cc: peterz, acme, jolsa, namhyung, bp, x86, linux-perf-users, linux-kernel, sandipan.das, ananth.narayan, santosh.shukla, Ravi Bangoria > Sorry Ravi, I've got my head in the clouds. I was reading this as a > tools patch :-) No worries :). And thanks for taking a look at the patch! Ravi ^ permalink raw reply [flat|nested] 14+ messages in thread
* [tip: perf/core] perf/amd: Prevent grouping of IBS events 2023-06-20 9:16 [PATCH] perf/amd: Prevent grouping of IBS events Ravi Bangoria 2023-06-20 16:44 ` Ian Rogers @ 2023-07-10 8:37 ` tip-bot2 for Ravi Bangoria 1 sibling, 0 replies; 14+ messages in thread From: tip-bot2 for Ravi Bangoria @ 2023-07-10 8:37 UTC (permalink / raw) To: linux-tip-commits Cc: Sandipan Das, Ravi Bangoria, Peter Zijlstra (Intel), x86, linux-kernel The following commit has been merged into the perf/core branch of tip: Commit-ID: 7c2128235eff99b448af8f4b5b2933495bf1a440 Gitweb: https://git.kernel.org/tip/7c2128235eff99b448af8f4b5b2933495bf1a440 Author: Ravi Bangoria <ravi.bangoria@amd.com> AuthorDate: Tue, 20 Jun 2023 14:46:03 +05:30 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Mon, 10 Jul 2023 09:52:34 +02:00 perf/amd: Prevent grouping of IBS events IBS PMUs can have only one event active at any point in time. Restrict grouping of multiple IBS events. Reported-by: Sandipan Das <sandipan.das@amd.com> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/20230620091603.269-1-ravi.bangoria@amd.com --- arch/x86/events/amd/ibs.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c index 3710148..74e6642 100644 --- a/arch/x86/events/amd/ibs.c +++ b/arch/x86/events/amd/ibs.c @@ -247,11 +247,33 @@ int forward_event_to_ibs(struct perf_event *event) return -ENOENT; } +/* + * Grouping of IBS events is not possible since IBS can have only + * one event active at any point in time. + */ +static int validate_group(struct perf_event *event) +{ + struct perf_event *sibling; + + if (event->group_leader == event) + return 0; + + if (event->group_leader->pmu == event->pmu) + return -EINVAL; + + for_each_sibling_event(sibling, event->group_leader) { + if (sibling->pmu == event->pmu) + return -EINVAL; + } + return 0; +} + static int perf_ibs_init(struct perf_event *event) { struct hw_perf_event *hwc = &event->hw; struct perf_ibs *perf_ibs; u64 max_cnt, config; + int ret; perf_ibs = get_ibs_pmu(event->attr.type); if (!perf_ibs) @@ -265,6 +287,10 @@ static int perf_ibs_init(struct perf_event *event) if (config & ~perf_ibs->config_mask) return -EINVAL; + ret = validate_group(event); + if (ret) + return ret; + if (hwc->sample_period) { if (config & perf_ibs->cnt_mask) /* raw max_cnt may not be set */ ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-07-10 8:38 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-20 9:16 [PATCH] perf/amd: Prevent grouping of IBS events Ravi Bangoria 2023-06-20 16:44 ` Ian Rogers 2023-06-21 3:27 ` Ravi Bangoria 2023-06-21 6:23 ` [PATCH] perf evsel amd: Fix IBS error message Ravi Bangoria 2023-06-21 22:33 ` Namhyung Kim 2023-06-22 5:27 ` Ravi Bangoria 2023-06-22 5:38 ` Namhyung Kim 2023-06-21 22:50 ` [PATCH] perf/amd: Prevent grouping of IBS events Namhyung Kim 2023-06-22 3:30 ` Ravi Bangoria 2023-06-22 5:09 ` Ian Rogers 2023-06-22 5:39 ` Ravi Bangoria 2023-06-22 5:44 ` Ian Rogers 2023-06-22 5:49 ` Ravi Bangoria 2023-07-10 8:37 ` [tip: perf/core] " tip-bot2 for Ravi Bangoria
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).