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@xxxxxx>
>> To: Ian Rogers <irogers@xxxxxxxxxx>
>> To: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
>> Signed-off-by: Thomas Richter <tmricht@xxxxxxxxxxxxx>
>> Suggested-by: Jiri Olsa <jolsa@xxxxxxxxxx>
>> ---
>> 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
