Re: [RFC PATCH bpf-next/net v1 07/13] bpf: Introduce per-prog stdout/stderr streams

[Eduard Zingerman <eddyz87@gmail.com>]

Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:

[...]

> > > +BTF_KFUNCS_START(stream_consumer_kfunc_set)
> > > +BTF_ID_FLAGS(func, bpf_stream_next_elem_batch, KF_ACQUIRE | KF_RET_NULL | KF_TRUSTED_ARGS)
> > > +BTF_ID_FLAGS(func, bpf_stream_free_elem_batch, KF_RELEASE)
> > > +BTF_ID_FLAGS(func, bpf_stream_next_elem, KF_ACQUIRE | KF_RET_NULL | KF_TRUSTED_ARGS)
> > > +BTF_ID_FLAGS(func, bpf_stream_free_elem, KF_RELEASE)
> > > +BTF_ID_FLAGS(func, bpf_prog_stream_get, KF_ACQUIRE | KF_RET_NULL)
> > > +BTF_ID_FLAGS(func, bpf_prog_stream_put, KF_RELEASE)
> > > +BTF_KFUNCS_END(stream_consumer_kfunc_set)
> >
> > This is a complicated API.
> > If we anticipate that users intend to write this info to ring buffers
> > maybe just provide a function doing that and do not expose complete API?
>
> I don't think anyone will use these functions directly, though they
> can if they want to.
> It's meant to be hidden behind bpftool, and macros to print stuff like
> bpf_printk().
>
> We cannot pop one message at a time, since they are not in FIFO order.
> So we need to splice out the whole batch queued and reverse it, before
> popping things.
> It's a consequence of using lockless lists.
>
> The other option is using a lock to protect the list, but using
> rqspinlock to then report messages about rqspinlock sounds like a
> circular dependency.

The API exposes 6 kfuncs and 3 data structures.
If things are exposed these things would be used, I wouldn't assume
that bpftool would be the only user. I'm sure people would craft their
own monitoring solutions.

Imo, exposing 9 entities is a lot. What I don't like about it,
is that API is not abstract:
- user cares about getting program log to some buffer, stream or file;
- user does not care how the strings are split internally in order to
  make logging efficient.

If at some point we decide to change implementation this API would be
hard to preserve.

Hence I suggest to instead provide a set of kfuncs that would drain
message queue into user provided ringbuf or buffer, w/o exposing
details.

A question: why exposing this functionality as kfuncs and not BPF
syscall commands?