On Thu, Feb 2, 2023 at 11:14 PM Jon Doron <arilou@xxxxxxxxx> wrote:
>
> On 02/02/2023, Andrii Nakryiko wrote:
> >On Wed, Feb 1, 2023 at 10:26 PM Jon Doron <arilou@xxxxxxxxx> wrote:
> >>
> >> From: Jon Doron <jond@xxxxxx>
> >>
> >> Add option to set when the perf buffer should wake up, by default the
> >> perf buffer becomes signaled for every event that is being pushed to it.
> >>
> >> In case of a high throughput of events it will be more efficient to wake
> >> up only once you have X events ready to be read.
> >>
> >> So your application can wakeup once and drain the entire perf buffer.
> >>
> >> Signed-off-by: Jon Doron <jond@xxxxxx>
> >> ---
> >> tools/lib/bpf/libbpf.c | 4 ++--
> >> tools/lib/bpf/libbpf.h | 3 ++-
> >> 2 files changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index eed5cec6f510..6b30ff13922b 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -11719,8 +11719,8 @@ struct perf_buffer *perf_buffer__new(int map_fd, size_t page_cnt,
> >> attr.config = PERF_COUNT_SW_BPF_OUTPUT;
> >> attr.type = PERF_TYPE_SOFTWARE;
> >> attr.sample_type = PERF_SAMPLE_RAW;
> >> - attr.sample_period = 1;
> >> - attr.wakeup_events = 1;
> >> + attr.sample_period = OPTS_GET(opts, wakeup_events, 1);
> >> + attr.wakeup_events = OPTS_GET(opts, wakeup_events, 1);
> >
> >I suspect the case of
> >
> >LIBBPF_OPTS(perf_buffer_opts, opts);
> >
> >perf_buffer__new(...., &opts);
> >
> >is not handled correctly and you end up with sample_period == wakeup_events == 0
> >
> >Can you please add BPF selftests that's setting wakeup_events to zero
> >and separately to >1?
> >
>
> Hi Andrii,
>
> I'm not sure what we are testing, when you have sample_period ==
> wakeup_events == 0, it basically means to never wakeup, so let's say you
> would wait on the poll_fd infinitely it will never wake you up.
>
> When you have let's say wakeup_event != 0, you will wakeup after the
> ring buffer in the perf buffer has more events than wakeup_events.
>
> I do see your point that if someone is using the macro to build the opts
> they will end with something unexpected, would you like me to treat 0 as
> 1 in that case?
Yes, exactly, I think we should treat zero as 1 and write a test that
this happens. Otherwise it will be very confusing when someone use
perf_buffer_opts for some other future option, and then suddenly
starts getting no notification. If someone really needs wakeup == 0,
they have a fallback plan to use perf_buffer__new_raw_opts(), which is
probably justified for some very specific and advanced uses.
So yes, please add a test with few subtests where we test default opts
(wakeup_after == 1), and wakeup_after > 1.
>
> -- Jon.
>
> >>
> >> p.attr = &attr;
> >> p.sample_cb = sample_cb;
> >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> >> index 8777ff21ea1d..e83c0a915dc7 100644
> >> --- a/tools/lib/bpf/libbpf.h
> >> +++ b/tools/lib/bpf/libbpf.h
> >> @@ -1246,8 +1246,9 @@ typedef void (*perf_buffer_lost_fn)(void *ctx, int cpu, __u64 cnt);
> >> /* common use perf buffer options */
> >> struct perf_buffer_opts {
> >> size_t sz;
> >> + __u32 wakeup_events;
> >> };
> >> -#define perf_buffer_opts__last_field sz
> >> +#define perf_buffer_opts__last_field wakeup_events
> >>
> >> /**
> >> * @brief **perf_buffer__new()** creates BPF perfbuf manager for a specified
> >> --
> >> 2.39.1
> >>
