On Fri, 9 May 2025 at 01:42, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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 ?
We can take the stream id directly in bpf_stream_vprintk, we have room
for one more argument, that can be hidden prog->aux.
Then we can drop bpf_stream_get.
Alternatively there's a way to call it bpf_stream_self.
I wasn't concerned about inconsistency since bpf_stream_vprintk is not
something people will use directly, you have to stuff arguments as array
of u64 etc. so it's unusable in practice. The main API exposed is
bpf_stream_printk. But I get the concern.
