On Wed, May 7, 2025 at 10:17 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> Introduce a new macro that allows printing data similar to bpf_printk(),
> but to BPF streams. The first argument is the stream ID, the rest of the
> arguments are same as what one would pass to bpf_printk().
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> kernel/bpf/stream.c | 10 +++++++--
> tools/lib/bpf/bpf_helpers.h | 44 +++++++++++++++++++++++++++++++------
> 2 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> index eaf0574866b1..d64975486ad1 100644
> --- a/kernel/bpf/stream.c
> +++ b/kernel/bpf/stream.c
> @@ -257,7 +257,12 @@ __bpf_kfunc int bpf_stream_vprintk(struct bpf_stream *stream, const char *fmt__s
> return ret;
> }
>
> -__bpf_kfunc struct bpf_stream *bpf_stream_get(enum bpf_stream_id stream_id, void *aux__ign)
> +/* Use int vs enum stream_id here, we use this kfunc in bpf_helpers.h, and
> + * keeping enum stream_id necessitates a complete definition of enum, but we
> + * can't copy it in the header as it may conflict with the definition in
> + * vmlinux.h.
> + */
> +__bpf_kfunc struct bpf_stream *bpf_stream_get(int stream_id, void *aux__ign)
> {
> struct bpf_prog_aux *aux = aux__ign;
>
> @@ -351,7 +356,8 @@ __bpf_kfunc struct bpf_stream_elem *bpf_stream_next_elem(struct bpf_stream *stre
> return elem;
> }
>
> -__bpf_kfunc struct bpf_stream *bpf_prog_stream_get(enum bpf_stream_id stream_id, u32 prog_id)
> +/* Use int vs enum bpf_stream_id for consistency with bpf_stream_get. */
> +__bpf_kfunc struct bpf_stream *bpf_prog_stream_get(int stream_id, u32 prog_id)
> {
> struct bpf_stream *stream;
> struct bpf_prog *prog;
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index a50773d4616e..1a748c21e358 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -314,17 +314,47 @@ enum libbpf_tristate {
> ___param, sizeof(___param)); \
> })
>
> +struct bpf_stream;
> +
> +extern struct bpf_stream *bpf_stream_get(int stream_id, void *aux__ign) __weak __ksym;
> +extern int bpf_stream_vprintk(struct bpf_stream *stream, const char *fmt__str, const void *args,
> + __u32 len__sz) __weak __ksym;
> +
> +#define __bpf_stream_vprintk(stream, fmt, args...) \
> +({ \
> + static const char ___fmt[] = fmt; \
> + unsigned long long ___param[___bpf_narg(args)]; \
> + \
> + _Pragma("GCC diagnostic push") \
> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + ___bpf_fill(___param, args); \
> + _Pragma("GCC diagnostic pop") \
> + \
> + int ___id = stream; \
> + struct bpf_stream *___sptr = bpf_stream_get(___id, NULL); \
> + if (___sptr) \
> + bpf_stream_vprintk(___sptr, ___fmt, ___param, sizeof(___param));\
> +})
Typically _get() is an acquire kfunc,
but here:
+BTF_ID_FLAGS(func, bpf_stream_get, KF_RET_NULL)
...
+BTF_ID_FLAGS(func, bpf_prog_stream_get, KF_ACQUIRE | KF_RET_NULL)
This is odd and it makes above sequence look weird too.
This is inconsistent as well:
bpf_stream_printk(int stream,
bpf_stream_vprintk(struct bpf_stream *stream,
Existing helpers bpf_trace_printk() and bpf_trace_vprintk()
are consistent.
Not sure why bpf_stream_get() is needed at all.
Maybe
#define BPF_STDOUT ((struct bpf_stream *)1)
#define BPF_STDERR ((struct bpf_stream *)2)
not pretty, but at least api will be consistent.
Other ideas ?
