IBS PMUs can have only one event active at any point in time. Restrict
grouping of multiple IBS events.
Reported-by: Sandipan Das <[email protected]>
Signed-off-by: Ravi Bangoria <[email protected]>
---
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
On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <[email protected]> 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 <[email protected]>
> Signed-off-by: Ravi Bangoria <[email protected]>
> ---
> 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
>
Hi Ian,
On 20-Jun-23 10:14 PM, Ian Rogers wrote:
> On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <[email protected]> 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
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 <[email protected]>
---
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
Hi Ravi,
On Tue, Jun 20, 2023 at 11:24 PM Ravi Bangoria <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Tue, Jun 20, 2023 at 8:28 PM Ravi Bangoria <[email protected]> wrote:
>
> Hi Ian,
>
> On 20-Jun-23 10:14 PM, Ian Rogers wrote:
> > On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <[email protected]> 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
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
On Wed, Jun 21, 2023 at 10:28 PM Ravi Bangoria <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
On Wed, Jun 21, 2023 at 10:39 PM Ravi Bangoria <[email protected]> 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
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
On Tue, Jun 20, 2023 at 8:27 PM Ravi Bangoria <[email protected]> wrote:
>
> Hi Ian,
>
> On 20-Jun-23 10:14 PM, Ian Rogers wrote:
> > On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <[email protected]> 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
> 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
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
AuthorDate: Tue, 20 Jun 2023 14:46:03 +05:30
Committer: Peter Zijlstra <[email protected]>
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 <[email protected]>
Signed-off-by: Ravi Bangoria <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
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 */