[PATCH v2] perf/s390: Regression: Move uid filtering to BPF filters

[PATCH v2] perf/s390: Regression: Move uid filtering to BPF filters

[Thomas Richter]

V1 --> V2: Added Jiri Olsa's suggestion and introduced
           member bpf_perf_event_opts::no_ioctl_enable.

On linux-next
commit b4c658d4d63d61 ("perf target: Remove uid from target")
introduces a regression on s390. In fact the regression exists
on all platforms when the event supports auxiliary data gathering.

Command
   # ./perf record -u 0 -aB --synth=no -- ./perf test -w thloop
   [ perf record: Woken up 1 times to write data ]
   [ perf record: Captured and wrote 0.011 MB perf.data ]
   # ./perf report --stats | grep SAMPLE
   #

does not generate samples in the perf.data file.
On x86 command
  # sudo perf record -e intel_pt// -u 0 ls
is broken too.

Looking at the sequence of calls in 'perf record' reveals this
behavior:
1. The event 'cycles' is created and enabled:
   record__open()
   +-> evlist__apply_filters()
       +-> perf_bpf_filter__prepare()
	   +-> bpf_program.attach_perf_event()
	       +-> bpf_program.attach_perf_event_opts()
	           +-> __GI___ioctl(..., PERF_EVENT_IOC_ENABLE, ...)
   The event 'cycles' is enabled and active now. However the event's
   ring-buffer to store the samples generated by hardware is not
   allocated yet. This happens now after enabling the event:

2. The event's fd is mmap() to create the ring buffer:
   record__open()
   +-> record__mmap()
       +-> record__mmap_evlist()
	   +-> evlist__mmap_ex()
	       +-> perf_evlist__mmap_ops()
	           +-> mmap_per_cpu()
	               +-> mmap_per_evsel()
	                   +-> mmap__mmap()
	                       +-> perf_mmap__mmap()
	                           +-> mmap()

   This allocates the ring-buffer for the event 'cycles'.  With mmap()
   the kernel creates the ring buffer:

   perf_mmap(): kernel function to create the event's ring
   |            buffer to save the sampled data.
   |
   +-> ring_buffer_attach(): Allocates memory for ring buffer.
       |        The PMU has auxiliary data setup function. The
       |        has_aux(event) condition is true and the PMU's
       |        stop() is called to stop sampling. It is not
       |        restarted:
       |        if (has_aux(event))
       |                perf_event_stop(event, 0);
       |
       +-> cpumsf_pmu_stop():

   Hardware sampling is stopped. No samples are generated and saved
   anymore.

3. After the event 'cycles' has been mapped, the event is enabled a
   second time in:
   __cmd_record()
   +-> evlist__enable()
       +-> __evlist__enable()
	   +-> evsel__enable_cpu()
	       +-> perf_evsel__enable_cpu()
	           +-> perf_evsel__run_ioctl()
	               +-> perf_evsel__ioctl()
	                   +-> __GI___ioctl(., PERF_EVENT_IOC_ENABLE, .)
   The second
      ioctl(fd, PERF_EVENT_IOC_ENABLE, 0);
   is just a NOP in this case. The first invocation in (1.) sets the
   event::state to PERF_EVENT_STATE_ACTIVE. The kernel functions
   perf_ioctl()
   +-> _perf_ioctl()
       +-> _perf_event_enable()
           +-> __perf_event_enable() returns immediately because
	              event::state is already set to
		      PERF_EVENT_STATE_ACTIVE.

This happens on s390, because the event 'cycles' offers the possibility
to save auxilary data. The PMU call backs setup_aux() and
free_aux() are defined. Without both call back functions,
cpumsf_pmu_stop() is not invoked and sampling continues.

To remedy this, remove the first invocation of
   ioctl(..., PERF_EVENT_IOC_ENABLE, ...).
in step (1.) Create the event in step (1.) and enable it in step (3.)
after the ring buffer has been mapped.
Make the change backward compatible and introduce a new structure
member bpf_perf_event_opts::no_ioctl_enable. It defaults to false and only
bpf_program__attach_perf_event() sets it to true. This way only
perf tool invocation do not enable the sampling event.

Output after:
 # ./perf record -aB --synth=no -u 0 -- ./perf test -w thloop 2
 [ perf record: Woken up 3 times to write data ]
 [ perf record: Captured and wrote 0.876 MB perf.data ]
 # ./perf  report --stats | grep SAMPLE
              SAMPLE events:      16200  (99.5%)
              SAMPLE events:      16200
 #

The software event succeeded before and after the patch:
 # ./perf record -e cpu-clock -aB --synth=no -u 0 -- \
					  ./perf test -w thloop 2
 [ perf record: Woken up 7 times to write data ]
 [ perf record: Captured and wrote 2.870 MB perf.data ]
 # ./perf  report --stats | grep SAMPLE
              SAMPLE events:      53506  (99.8%)
              SAMPLE events:      53506
 #

Fixes: 63f2f5ee856ba ("libbpf: add ability to attach/detach BPF program to perf event")
To: Andrii Nakryiko <andriin@fb.com>
To: Ian Rogers <irogers@google.com>
To: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Suggested-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/lib/bpf/libbpf.c | 19 +++++++++++++------
 tools/lib/bpf/libbpf.h |  3 ++-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e067cb5776bd..8e0fb4391b54 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10911,6 +10911,7 @@ struct bpf_link *bpf_program__attach_perf_event_opts(const struct bpf_program *p
 	struct bpf_link_perf *link;
 	int prog_fd, link_fd = -1, err;
 	bool force_ioctl_attach;
+	bool no_ioctl_enable;
 
 	if (!OPTS_VALID(opts, bpf_perf_event_opts))
 		return libbpf_err_ptr(-EINVAL);
@@ -10965,11 +10966,14 @@ struct bpf_link *bpf_program__attach_perf_event_opts(const struct bpf_program *p
 		}
 		link->link.fd = pfd;
 	}
-	if (ioctl(pfd, PERF_EVENT_IOC_ENABLE, 0) < 0) {
-		err = -errno;
-		pr_warn("prog '%s': failed to enable perf_event FD %d: %s\n",
-			prog->name, pfd, errstr(err));
-		goto err_out;
+	no_ioctl_enable = OPTS_GET(opts, no_ioctl_enable, false);
+	if (!no_ioctl_enable) {
+		if (ioctl(pfd, PERF_EVENT_IOC_ENABLE, 0) < 0) {
+			err = -errno;
+			pr_warn("prog '%s': failed to enable perf_event FD %d: %s\n",
+				prog->name, pfd, errstr(err));
+			goto err_out;
+		}
 	}
 
 	return &link->link;
@@ -10982,7 +10986,10 @@ struct bpf_link *bpf_program__attach_perf_event_opts(const struct bpf_program *p
 
 struct bpf_link *bpf_program__attach_perf_event(const struct bpf_program *prog, int pfd)
 {
-	return bpf_program__attach_perf_event_opts(prog, pfd, NULL);
+	DECLARE_LIBBPF_OPTS(bpf_perf_event_opts, pe_opts);
+
+	pe_opts.no_ioctl_enable = true;
+	return bpf_program__attach_perf_event_opts(prog, pfd, &pe_opts);
 }
 
 /*
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index d1cf813a057b..4be2b7664031 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -499,9 +499,10 @@ struct bpf_perf_event_opts {
 	__u64 bpf_cookie;
 	/* don't use BPF link when attach BPF program */
 	bool force_ioctl_attach;
+	bool no_ioctl_enable;
 	size_t :0;
 };
-#define bpf_perf_event_opts__last_field force_ioctl_attach
+#define bpf_perf_event_opts__last_field no_ioctl_enable
 
 LIBBPF_API struct bpf_link *
 bpf_program__attach_perf_event(const struct bpf_program *prog, int pfd);
-- 
2.50.1

Re: [PATCH v2] perf/s390: Regression: Move uid filtering to BPF filters

[Ilya Leoshkevich]

On Mon, 2025-07-28 at 16:43 +0200, Thomas Richter wrote:
> V1 --> V2: Added Jiri Olsa's suggestion and introduced
>            member bpf_perf_event_opts::no_ioctl_enable.
> 
> On linux-next
> commit b4c658d4d63d61 ("perf target: Remove uid from target")
> introduces a regression on s390. In fact the regression exists
> on all platforms when the event supports auxiliary data gathering.
> 
> Command
>    # ./perf record -u 0 -aB --synth=no -- ./perf test -w thloop
>    [ perf record: Woken up 1 times to write data ]
>    [ perf record: Captured and wrote 0.011 MB perf.data ]
>    # ./perf report --stats | grep SAMPLE
>    #
> 
> does not generate samples in the perf.data file.
> On x86 command
>   # sudo perf record -e intel_pt// -u 0 ls
> is broken too.
> 
> Looking at the sequence of calls in 'perf record' reveals this
> behavior:
> 1. The event 'cycles' is created and enabled:
>    record__open()
>    +-> evlist__apply_filters()
>        +-> perf_bpf_filter__prepare()
> 	   +-> bpf_program.attach_perf_event()
> 	       +-> bpf_program.attach_perf_event_opts()
> 	           +-> __GI___ioctl(..., PERF_EVENT_IOC_ENABLE, ...)
>    The event 'cycles' is enabled and active now. However the event's
>    ring-buffer to store the samples generated by hardware is not
>    allocated yet. This happens now after enabling the event:
> 
> 2. The event's fd is mmap() to create the ring buffer:
>    record__open()
>    +-> record__mmap()
>        +-> record__mmap_evlist()
> 	   +-> evlist__mmap_ex()
> 	       +-> perf_evlist__mmap_ops()
> 	           +-> mmap_per_cpu()
> 	               +-> mmap_per_evsel()
> 	                   +-> mmap__mmap()
> 	                       +-> perf_mmap__mmap()
> 	                           +-> mmap()
> 
>    This allocates the ring-buffer for the event 'cycles'.  With
> mmap()
>    the kernel creates the ring buffer:
> 
>    perf_mmap(): kernel function to create the event's ring
>    |            buffer to save the sampled data.
>    |
>    +-> ring_buffer_attach(): Allocates memory for ring buffer.
>        |        The PMU has auxiliary data setup function. The
>        |        has_aux(event) condition is true and the PMU's
>        |        stop() is called to stop sampling. It is not
>        |        restarted:
>        |        if (has_aux(event))
>        |                perf_event_stop(event, 0);
>        |
>        +-> cpumsf_pmu_stop():
> 
>    Hardware sampling is stopped. No samples are generated and saved
>    anymore.
> 
> 3. After the event 'cycles' has been mapped, the event is enabled a
>    second time in:
>    __cmd_record()
>    +-> evlist__enable()
>        +-> __evlist__enable()
> 	   +-> evsel__enable_cpu()
> 	       +-> perf_evsel__enable_cpu()
> 	           +-> perf_evsel__run_ioctl()
> 	               +-> perf_evsel__ioctl()
> 	                   +-> __GI___ioctl(.,
> PERF_EVENT_IOC_ENABLE, .)
>    The second
>       ioctl(fd, PERF_EVENT_IOC_ENABLE, 0);
>    is just a NOP in this case. The first invocation in (1.) sets the
>    event::state to PERF_EVENT_STATE_ACTIVE. The kernel functions
>    perf_ioctl()
>    +-> _perf_ioctl()
>        +-> _perf_event_enable()
>            +-> __perf_event_enable() returns immediately because
> 	              event::state is already set to
> 		      PERF_EVENT_STATE_ACTIVE.
> 
> This happens on s390, because the event 'cycles' offers the
> possibility
> to save auxilary data. The PMU call backs setup_aux() and
> free_aux() are defined. Without both call back functions,
> cpumsf_pmu_stop() is not invoked and sampling continues.
> 
> To remedy this, remove the first invocation of
>    ioctl(..., PERF_EVENT_IOC_ENABLE, ...).
> in step (1.) Create the event in step (1.) and enable it in step (3.)
> after the ring buffer has been mapped.
> Make the change backward compatible and introduce a new structure
> member bpf_perf_event_opts::no_ioctl_enable. It defaults to false and
> only
> bpf_program__attach_perf_event() sets it to true. This way only
> perf tool invocation do not enable the sampling event.
> 
> Output after:
>  # ./perf record -aB --synth=no -u 0 -- ./perf test -w thloop 2
>  [ perf record: Woken up 3 times to write data ]
>  [ perf record: Captured and wrote 0.876 MB perf.data ]
>  # ./perf  report --stats | grep SAMPLE
>               SAMPLE events:      16200  (99.5%)
>               SAMPLE events:      16200
>  #
> 
> The software event succeeded before and after the patch:
>  # ./perf record -e cpu-clock -aB --synth=no -u 0 -- \
> 					  ./perf test -w thloop 2
>  [ perf record: Woken up 7 times to write data ]
>  [ perf record: Captured and wrote 2.870 MB perf.data ]
>  # ./perf  report --stats | grep SAMPLE
>               SAMPLE events:      53506  (99.8%)
>               SAMPLE events:      53506
>  #
> 
> Fixes: 63f2f5ee856ba ("libbpf: add ability to attach/detach BPF
> program to perf event")
> To: Andrii Nakryiko <andriin@fb.com>
> To: Ian Rogers <irogers@google.com>
> To: Ilya Leoshkevich <iii@linux.ibm.com>
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Suggested-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/lib/bpf/libbpf.c | 19 +++++++++++++------
>  tools/lib/bpf/libbpf.h |  3 ++-
>  2 files changed, 15 insertions(+), 7 deletions(-)

What do you think about rather calling the new field ioctl_enable?
So that we don't get double negations in the API users and
implementation - they are sometimes unnecessarily confusing.

I also think enablement should be the default in
bpf_program__attach_perf_event(), and perf should now call
bpf_program__attach_perf_event_opts() instead.

Based on your request in v1, I can offer to take over the patch and
send a v3 with the changes I suggested above.

Re: [PATCH v2] perf/s390: Regression: Move uid filtering to BPF filters

[Thomas Richter]

On 7/31/25 10:38, Ilya Leoshkevich wrote:
> On Mon, 2025-07-28 at 16:43 +0200, Thomas Richter wrote:
>> V1 --> V2: Added Jiri Olsa's suggestion and introduced
>>            member bpf_perf_event_opts::no_ioctl_enable.
>>
>> On linux-next
>> commit b4c658d4d63d61 ("perf target: Remove uid from target")
>> introduces a regression on s390. In fact the regression exists
>> on all platforms when the event supports auxiliary data gathering.
>>
>> Command
>>    # ./perf record -u 0 -aB --synth=no -- ./perf test -w thloop
>>    [ perf record: Woken up 1 times to write data ]
>>    [ perf record: Captured and wrote 0.011 MB perf.data ]
>>    # ./perf report --stats | grep SAMPLE
>>    #
>>
>> does not generate samples in the perf.data file.
>> On x86 command
>>   # sudo perf record -e intel_pt// -u 0 ls
>> is broken too.
>>
>> Looking at the sequence of calls in 'perf record' reveals this
>> behavior:
>> 1. The event 'cycles' is created and enabled:
>>    record__open()
>>    +-> evlist__apply_filters()
>>        +-> perf_bpf_filter__prepare()
>> 	   +-> bpf_program.attach_perf_event()
>> 	       +-> bpf_program.attach_perf_event_opts()
>> 	           +-> __GI___ioctl(..., PERF_EVENT_IOC_ENABLE, ...)
>>    The event 'cycles' is enabled and active now. However the event's
>>    ring-buffer to store the samples generated by hardware is not
>>    allocated yet. This happens now after enabling the event:
>>
>> 2. The event's fd is mmap() to create the ring buffer:
>>    record__open()
>>    +-> record__mmap()
>>        +-> record__mmap_evlist()
>> 	   +-> evlist__mmap_ex()
>> 	       +-> perf_evlist__mmap_ops()
>> 	           +-> mmap_per_cpu()
>> 	               +-> mmap_per_evsel()
>> 	                   +-> mmap__mmap()
>> 	                       +-> perf_mmap__mmap()
>> 	                           +-> mmap()
>>
>>    This allocates the ring-buffer for the event 'cycles'.  With
>> mmap()
>>    the kernel creates the ring buffer:
>>
>>    perf_mmap(): kernel function to create the event's ring
>>    |            buffer to save the sampled data.
>>    |
>>    +-> ring_buffer_attach(): Allocates memory for ring buffer.
>>        |        The PMU has auxiliary data setup function. The
>>        |        has_aux(event) condition is true and the PMU's
>>        |        stop() is called to stop sampling. It is not
>>        |        restarted:
>>        |        if (has_aux(event))
>>        |                perf_event_stop(event, 0);
>>        |
>>        +-> cpumsf_pmu_stop():
>>
>>    Hardware sampling is stopped. No samples are generated and saved
>>    anymore.
>>
>> 3. After the event 'cycles' has been mapped, the event is enabled a
>>    second time in:
>>    __cmd_record()
>>    +-> evlist__enable()
>>        +-> __evlist__enable()
>> 	   +-> evsel__enable_cpu()
>> 	       +-> perf_evsel__enable_cpu()
>> 	           +-> perf_evsel__run_ioctl()
>> 	               +-> perf_evsel__ioctl()
>> 	                   +-> __GI___ioctl(.,
>> PERF_EVENT_IOC_ENABLE, .)
>>    The second
>>       ioctl(fd, PERF_EVENT_IOC_ENABLE, 0);
>>    is just a NOP in this case. The first invocation in (1.) sets the
>>    event::state to PERF_EVENT_STATE_ACTIVE. The kernel functions
>>    perf_ioctl()
>>    +-> _perf_ioctl()
>>        +-> _perf_event_enable()
>>            +-> __perf_event_enable() returns immediately because
>> 	              event::state is already set to
>> 		      PERF_EVENT_STATE_ACTIVE.
>>
>> This happens on s390, because the event 'cycles' offers the
>> possibility
>> to save auxilary data. The PMU call backs setup_aux() and
>> free_aux() are defined. Without both call back functions,
>> cpumsf_pmu_stop() is not invoked and sampling continues.
>>
>> To remedy this, remove the first invocation of
>>    ioctl(..., PERF_EVENT_IOC_ENABLE, ...).
>> in step (1.) Create the event in step (1.) and enable it in step (3.)
>> after the ring buffer has been mapped.
>> Make the change backward compatible and introduce a new structure
>> member bpf_perf_event_opts::no_ioctl_enable. It defaults to false and
>> only
>> bpf_program__attach_perf_event() sets it to true. This way only
>> perf tool invocation do not enable the sampling event.
>>
>> Output after:
>>  # ./perf record -aB --synth=no -u 0 -- ./perf test -w thloop 2
>>  [ perf record: Woken up 3 times to write data ]
>>  [ perf record: Captured and wrote 0.876 MB perf.data ]
>>  # ./perf  report --stats | grep SAMPLE
>>               SAMPLE events:      16200  (99.5%)
>>               SAMPLE events:      16200
>>  #
>>
>> The software event succeeded before and after the patch:
>>  # ./perf record -e cpu-clock -aB --synth=no -u 0 -- \
>> 					  ./perf test -w thloop 2
>>  [ perf record: Woken up 7 times to write data ]
>>  [ perf record: Captured and wrote 2.870 MB perf.data ]
>>  # ./perf  report --stats | grep SAMPLE
>>               SAMPLE events:      53506  (99.8%)
>>               SAMPLE events:      53506
>>  #
>>
>> Fixes: 63f2f5ee856ba ("libbpf: add ability to attach/detach BPF
>> program to perf event")
>> To: Andrii Nakryiko <andriin@fb.com>
>> To: Ian Rogers <irogers@google.com>
>> To: Ilya Leoshkevich <iii@linux.ibm.com>
>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>> Suggested-by: Jiri Olsa <jolsa@redhat.com>
>> ---
>>  tools/lib/bpf/libbpf.c | 19 +++++++++++++------
>>  tools/lib/bpf/libbpf.h |  3 ++-
>>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> What do you think about rather calling the new field ioctl_enable?
> So that we don't get double negations in the API users and
> implementation - they are sometimes unnecessarily confusing.
> 
> I also think enablement should be the default in
> bpf_program__attach_perf_event(), and perf should now call
> bpf_program__attach_perf_event_opts() instead.
> 
> Based on your request in v1, I can offer to take over the patch and
> send a v3 with the changes I suggested above.

Yes Ilya, please go ahead.
Thanks very much for your support.

-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
IBM Deutschland Research & Development GmbH

Vorsitzender des Aufsichtsrats: Wolfgang Wendt

Geschäftsführung: David Faller

Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294