On Wed, 23 Apr 2025 at 00:10, Quentin Monnet <qmo@xxxxxxxxxx> wrote:
>
> 2025-04-14 09:14 UTC-0700 ~ Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > Add bpftool support for dumping streams of a given BPF program.
>
>
> Thanks for adding bpftool support!
>
>
> > TODO: JSON and filepath support.
>
>
> ... plus documentation (man page), and bash completion please.
>
Done.
>
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > tools/bpf/bpftool/Makefile | 2 +-
> > tools/bpf/bpftool/prog.c | 71 +++++++++++++++++-
> > tools/bpf/bpftool/skeleton/stream.bpf.c | 96 +++++++++++++++++++++++++
> > 3 files changed, 166 insertions(+), 3 deletions(-)
> > create mode 100644 tools/bpf/bpftool/skeleton/stream.bpf.c
> >
> > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > index 9e9a5f006cd2..eb908223c3bb 100644
> > --- a/tools/bpf/bpftool/Makefile
> > +++ b/tools/bpf/bpftool/Makefile
> > @@ -234,7 +234,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF_BOOTSTRAP)
> > $(OUTPUT)%.skel.h: $(OUTPUT)%.bpf.o $(BPFTOOL_BOOTSTRAP)
> > $(QUIET_GEN)$(BPFTOOL_BOOTSTRAP) gen skeleton $< > $@
> >
> > -$(OUTPUT)prog.o: $(OUTPUT)profiler.skel.h
> > +$(OUTPUT)prog.o: $(OUTPUT)profiler.skel.h $(OUTPUT)stream.skel.h
> >
> > $(OUTPUT)pids.o: $(OUTPUT)pid_iter.skel.h
> >
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index f010295350be..d0800fec9c3d 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -35,12 +35,16 @@
> > #include "main.h"
> > #include "xlated_dumper.h"
> >
> > +#include "stream.skel.h"
> > +
> > #define BPF_METADATA_PREFIX "bpf_metadata_"
> > #define BPF_METADATA_PREFIX_LEN (sizeof(BPF_METADATA_PREFIX) - 1)
> >
> > enum dump_mode {
> > DUMP_JITED,
> > DUMP_XLATED,
> > + DUMP_STDOUT,
> > + DUMP_STDERR,
> > };
> >
> > static const bool attach_types[] = {
> > @@ -697,6 +701,55 @@ static int do_show(int argc, char **argv)
> > return err;
> > }
> >
> > +static int process_stream_sample(void *ctx, void *data, size_t len)
> > +{
> > + FILE *file = ctx;
> > +
> > + fprintf(file, "%s", (char *)data);
> > + fflush(file);
> > + return 0;
> > +}
> > +
> > +static int
> > +prog_dump_stream(struct bpf_prog_info *info, enum dump_mode mode, const char *filepath)
> > +{
> > + FILE *file = mode == DUMP_STDOUT ? stdout : stderr;
> > + LIBBPF_OPTS(bpf_test_run_opts, opts);
> > + struct ring_buffer *ringbuf;
> > + struct stream_bpf *skel;
> > + int map_fd, ret = -1;
> > +
> > + __u32 prog_id = info->id;
> > + __u32 stream_id = mode == DUMP_STDOUT ? 1 : 2;
> > +
> > + skel = stream_bpf__open_and_load();
> > + if (!skel)
> > + return -errno;
> > + skel->bss->prog_id = prog_id;
> > + skel->bss->stream_id = stream_id;
> > +
> > + //TODO(kkd): Filepath handling
> > + map_fd = bpf_map__fd(skel->maps.ringbuf);
> > + ringbuf = ring_buffer__new(map_fd, process_stream_sample, file, NULL);
> > + if (!ringbuf) {
> > + ret = -errno;
> > + goto end;
> > + }
> > + do {
> > + skel->bss->written_count = skel->bss->written_size = 0;
> > + ret = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.bpftool_dump_prog_stream), &opts);
> > + ret = -EINVAL;
> > + if (ring_buffer__consume_n(ringbuf, skel->bss->written_count) != skel->bss->written_count)
> > + goto end;
> > + } while (!ret && opts.retval == EAGAIN);
> > +
> > + if (opts.retval != 0)
> > + ret = -EINVAL;
> > +end:
> > + stream_bpf__destroy(skel);
> > + return ret;
> > +}
> > +
> > static int
> > prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
> > char *filepath, bool opcodes, bool visual, bool linum)
> > @@ -719,13 +772,15 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
> > }
> > buf = u64_to_ptr(info->jited_prog_insns);
> > member_len = info->jited_prog_len;
> > - } else { /* DUMP_XLATED */
> > + } else if (mode == DUMP_XLATED) { /* DUMP_XLATED */
> > if (info->xlated_prog_len == 0 || !info->xlated_prog_insns) {
> > p_err("error retrieving insn dump: kernel.kptr_restrict set?");
> > return -1;
> > }
> > buf = u64_to_ptr(info->xlated_prog_insns);
> > member_len = info->xlated_prog_len;
> > + } else if (mode == DUMP_STDOUT || mode == DUMP_STDERR) {
> > + return prog_dump_stream(info, mode, filepath);
> > }
> >
> > if (info->btf_id) {
> > @@ -898,8 +953,10 @@ static int do_dump(int argc, char **argv)
> > mode = DUMP_JITED;
> > } else if (is_prefix(*argv, "xlated")) {
> > mode = DUMP_XLATED;
> > + } else if (is_prefix(*argv, "stdout") || is_prefix(*argv, "stderr")) {
> > + mode = is_prefix(*argv, "stdout") ? DUMP_STDOUT : DUMP_STDERR;
> > } else {
> > - p_err("expected 'xlated' or 'jited', got: %s", *argv);
> > + p_err("expected 'stdout', 'stderr', 'xlated' or 'jited', got: %s", *argv);
> > return -1;
> > }
> > NEXT_ARG();
> > @@ -950,6 +1007,14 @@ static int do_dump(int argc, char **argv)
> > }
> > }
> >
> > + if (mode == DUMP_STDOUT || mode == DUMP_STDERR) {
> > + if (opcodes || visual || linum) {
> > + p_err("'%s' is not compatible with 'opcodes', 'visual', or 'linum'",
> > + mode == DUMP_STDOUT ? "stdout" : "stderr");
> > + goto exit_close;
> > + }
> > + }
> > +
> > if (filepath && (opcodes || visual || linum)) {
> > p_err("'file' is not compatible with 'opcodes', 'visual', or 'linum'");
> > goto exit_close;
> > @@ -2468,6 +2533,8 @@ static int do_help(int argc, char **argv)
> > "Usage: %1$s %2$s { show | list } [PROG]\n"
> > " %1$s %2$s dump xlated PROG [{ file FILE | [opcodes] [linum] [visual] }]\n"
> > " %1$s %2$s dump jited PROG [{ file FILE | [opcodes] [linum] }]\n"
> > + " %1$s %2$s dump stdout PROG [{ file FILE }]\n"
> > + " %1$s %2$s dump stderr PROG [{ file FILE }]\n"
>
>
> I'm not sure "prog dump" is the best subcommand for these features. The
> "dump" has been associated with printing the program itself, either
> translated or as JITed instructions. Here we display something else;
> it's closer to "prog tracelog", that we use to dump the trace pipe. And
> stdout/stderr are streams anyway, I'm not sure that "dumping" them is
> the most accurate term.
>
> How about "prog trace (stdout|stderr)"? Bonus: you don't have to handle
> opcodes/linum/visual, and don't have to add support for "filepath".
I went with prog tracelog {stdout|stderr}, since trace probably feels
a bit odd in this context.
Tracelog alone will print trace pipe, when passed stdout/stderr it
gives the stream output.
Let me know what you think.
>
>
> > " %1$s %2$s pin PROG FILE\n"
> > " %1$s %2$s { load | loadall } OBJ PATH \\\n"
> > " [type TYPE] [{ offload_dev | xdpmeta_dev } NAME] \\\n"
> > diff --git a/tools/bpf/bpftool/skeleton/stream.bpf.c b/tools/bpf/bpftool/skeleton/stream.bpf.c
> > new file mode 100644
> > index 000000000000..31b5933e0384
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/skeleton/stream.bpf.c
> > @@ -0,0 +1,96 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
>
> Bpftool is dual-licensed (GPL-2.0-only OR BSD-2-Clause), please consider
> using the same here.
Done.
>
>
> > +/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include <bpf/bpf_helpers.h>
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_RINGBUF);
> > + __uint(max_entries, 1024 * 1024);
> > +} ringbuf SEC(".maps");
> > +
> > +struct value {
> > + struct bpf_stream_elem_batch __kptr *batch;
> > +};
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __type(key, int);
> > + __type(value, struct value);
> > + __uint(max_entries, 1);
> > +} array SEC(".maps");
> > +
> > +int written_size;
> > +int written_count;
> > +int stream_id;
> > +int prog_id;
> > +
> > +#define ENOENT 2
> > +#define EAGAIN 11
> > +#define EFAULT 14
> > +
> > +SEC("syscall")
> > +int bpftool_dump_prog_stream(void *ctx)
> > +{
> > + struct bpf_stream_elem_batch *elem_batch;
> > + struct bpf_stream_elem *elem;
> > + struct bpf_stream *stream;
> > + bool cont = false;
> > + struct value *v;
> > + bool ret = 0;
> > +
> > + stream = bpf_prog_stream_get(BPF_STDERR, prog_id);
>
>
> Calls to these new kfuncs will break compilation on older systems that
> don't support them yet (and don't have the definition in their
> vmlinux.h). We should provide fallback definitions to make sure that the
> program compiles.
This is the only thing I haven't yet addressed in v2, because it
seemed a bit ugly.
I tried adding kfunc declarations, but those aren't enough.
We rely on structs introduced and read in this patch.
So I think vmlinux.h needs to be dropped, but it means adding a lot
more than just the declarations, all types, plus any types they
transitively depend on.
Maybe there is a better way (like detecting compilation failure and skipping?).
But if not, I will address like above in v3.
>
>
> > + if (!stream)
> > + return ENOENT;
> > +
> > + v = bpf_map_lookup_elem(&array, &(int){0});
> > +
> > + if (v->batch)
> > + elem_batch = bpf_kptr_xchg(&v->batch, NULL);
> > + else
> > + elem_batch = bpf_stream_next_elem_batch(stream);
> > + if (!elem_batch)
> > + goto end;
> > +
> > + bpf_repeat(BPF_MAX_LOOPS) {
> > + struct bpf_dynptr dst_dptr, src_dptr;
> > + int size;
> > +
> > + elem = bpf_stream_next_elem(elem_batch);
> > + if (!elem)
> > + break;
> > + size = elem->mem_slice.len;
> > +
> > + if (bpf_dynptr_from_mem_slice(&elem->mem_slice, 0, &src_dptr))
> > + ret = EFAULT;
> > + if (bpf_ringbuf_reserve_dynptr(&ringbuf, size, 0, &dst_dptr))
> > + ret = EFAULT;
> > + if (bpf_dynptr_copy(&dst_dptr, 0, &src_dptr, 0, size))
> > + ret = EFAULT;
> > + bpf_ringbuf_submit_dynptr(&dst_dptr, 0);
> > +
> > + written_count++;
> > + written_size += size;
> > +
> > + bpf_stream_free_elem(elem);
> > +
> > + /* Probe and exit if no more space, probe for twice the typical size.*/
> > + if (bpf_ringbuf_reserve_dynptr(&ringbuf, 2048, 0, &dst_dptr))
> > + cont = true;
> > + bpf_ringbuf_discard_dynptr(&dst_dptr, 0);
> > +
> > + if (ret || cont)
> > + break;
> > + }
> > +
> > + if (cont)
> > + elem_batch = bpf_kptr_xchg(&v->batch, elem_batch);
> > + if (elem_batch)
> > + bpf_stream_free_elem_batch(elem_batch);
> > +end:
> > + bpf_prog_stream_put(stream);
> > +
> > + return ret ?: (cont ? EAGAIN : 0);
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
>
>
> Same note on the license, other programs used by bpftool have license
> string "Dual BSD/GPL".
Done, please take a look at the changes in v2.
