On Fri, 9 May 2025 at 01:48, Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> 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.
Taking it directly does negate the ability to write into any stream *
one has access to, so there's that.
>
> 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.
