> Allow map event_pipe to consume BPF_MAP_TYPE_RINGBUF maps using
> libbpf's ring buffer manager. Print each record's size and raw bytes
> in plain or JSON output, and flush output as records arrive.
>
> Reject CPU and index selectors for ring buffers and retain the existing
> perf event array path. Keep signal handlers limited to setting a stop
> flag, reset that flag for each command, and print the stopping message
> on stderr from the main path. Close the JSON array on poll errors and
> preserve output errors returned by the ring buffer callback.
This isn't a bug, but could the changelog lead with why this is needed
(ring buffers are now the common output channel but event_pipe has no way
to read them) and keep only the parts of the mechanism that are not
obvious from the diff, such as the -EINTR return from the callback? The
second and third paragraphs walk through what individual hunks do rather
than explaining the rationale. A reader can learn what each hunk does but
not why the feature is wanted; the motivation is only reachable through
the Link: tag.
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst
> b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> index 5daf3de5c744..c44e6f797e57 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
[ ... ]
> @@ -134,6 +135,18 @@ bpftool map event_pipe *MAP* [cpu *N* index *M*]
> existing ring. Any other application will stop receiving events if it
> installed its rings earlier.
>
> + For a ring buffer map, consume records submitted by BPF programs,
> including
> + records already queued before the command starts. **cpu** and **index**
> + are not supported. Each record is printed as raw bytes, including
> embedded
> + zero bytes. Plain output reports the record size followed by hexadecimal
> + bytes; JSON output contains **size** and **data** fields, with **data**
> an
> + array of byte values. Ring buffer records have no implicit CPU or
> timestamp.
> +
> + Ring buffers support a single consumer. This command advances the shared
> + consumer position and must not run alongside another consumer of the same
> + map; it does not provide a passive view of events.
> **BPF_MAP_TYPE_USER_RINGBUF**
> + maps are not supported.
This isn't a bug, but would it read more tightly to state the
consumer-position effect once, e.g. "Consuming a ring buffer advances its
shared consumer position, so this command must not run alongside another
consumer of the same map", and to say "printed in full, including embedded
zero bytes" rather than "raw bytes" just before describing the hex output?
The single-consumer point is made four times in three sentences: "support
a single consumer", "advances the shared consumer position", "must not run
alongside another consumer of the same map", and "it does not provide a
passive view of events". The paragraph above has a similar redundancy in
the other direction: "Each record is printed as raw bytes, including
embedded zero bytes" is immediately followed by "Plain output reports the
record size followed by hexadecimal bytes", so "raw bytes" reads as a
contradiction of the hex encoding described one sentence later.
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool
> b/tools/bpf/bpftool/bash-completion/bpftool
> index 75cbcb512eba..1750b488c9e3 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -878,11 +878,11 @@ _bpftool()
> return 0
> ;;
> id)
> - _bpftool_get_map_ids_for_type perf_event_array
> + _bpftool_get_map_ids_for_type '"type":
> "\(perf_event_array\|ringbuf\)"'
> return 0
> ;;
> name)
> - _bpftool_get_map_names_for_type perf_event_array
> + _bpftool_get_map_names_for_type '"type":
> "\(perf_event_array\|ringbuf\)"'
> return 0
> ;;
> cpu)
This isn't a bug, but the other _for_type callers here pass a bare map
type (e.g. stack / queue on two lines). Would anchoring the "type": "$type"
match inside _bpftool_get_map_ids_for_type()/_bpftool_get_map_names_for_type()
and then calling them twice read more consistently than passing a grep
expression from the call site? The two new call sites pass a raw grep BRE
to helpers that every other caller in this file feeds a bare map type
name, and the call site now encodes the helper's internal implementation
detail (that it greps the pretty-printed JSON for a "type": "..." line).
The file's existing idiom for a two-type selection is two consecutive
calls (lines 682-683 and 700-701).
> diff --git a/tools/bpf/bpftool/map_perf_ring.c
> b/tools/bpf/bpftool/map_perf_ring.c
> index bcb767e2d673..7d555331f443 100644
> --- a/tools/bpf/bpftool/map_perf_ring.c
> +++ b/tools/bpf/bpftool/map_perf_ring.c
> @@ -27,7 +27,7 @@
>
> #define MMAP_PAGE_CNT 16
>
> -static volatile bool stop;
> +static volatile sig_atomic_t stop;
>
> struct perf_event_sample {
> struct perf_event_header header;
> @@ -44,7 +44,6 @@ struct perf_event_lost {
>
> static void int_exit(int signo)
> {
> - fprintf(stderr, "Stopping...\n");
> stop = true;
> }
Would it be worth splitting the signal-handler hardening (volatile bool ->
volatile sig_atomic_t, and moving the "Stopping..." print out of int_exit())
into a small preparatory patch? It looks like a standalone async-signal-safety
fix for the existing perf-event-array path, and separating it would make both
halves easier to review and backport. Two of the changes in this hunk are
async-signal-safety fixes to pre-existing code rather than part of the new
ring buffer support: widening stop from volatile bool to volatile sig_atomic_t,
and dropping the fprintf call out of the handler (fprintf is not
async-signal-safe). Both stand on their own and apply equally to the existing
perf-event-array path, whereas the rest of the patch (the stop = false reset,
the -EINTR return from the ring buffer callback, the jsonw_end_array() on the
error path) is genuinely needed for ring buffers.
[ ... ]
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34074185878
