Re: [PATCH bpf-next v1 2/3] bpf: Add support for KF_RET_RCU flag

[Andrea Righi <arighi@nvidia.com>]

On Mon, Sep 15, 2025 at 09:13:15AM +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 15 Sept 2025 at 08:14, Andrea Righi <arighi@nvidia.com> wrote:
> >
> > Hi Kumar,
> >
> > thanks for looking at this! Comment below.
> >
> > On Mon, Sep 15, 2025 at 02:47:30AM +0000, Kumar Kartikeya Dwivedi wrote:
> > > Add a kfunc annotation 'KF_RET_RCU' to signal that the return type must
> > > be marked MEM_RCU, to return objects that are RCU protected. Naturally,
> > > this must imply that the kfunc is invoked in an RCU critical section,
> > > and thus the presence of this flag implies the presence of the
> > > KF_RCU_PROTECTED flag. Upcoming kfunc scx_bpf_cpu_curr() [0] will be
> > > made to make use of this flag.
> >
> > I'm not sure we actually need two separate annotations, I can't think of a
> > case where KF_RCU_PROTECTED would be useful without also having KF_RET_RCU.
> >
> > What I mean is: if the kfunc is only meant to be called inside an RCU
> > critical section, but doesn't return an RCU-protected pointer, then we can
> > simply add rcu_read_lock/unlock() internally in the kfunc. And for kfuncs
> > that take RCU-protected arguments we already have KF_RCU.
> >
> > So it seems sufficient to have a single annotation that implements the
> > semantic "this kfunc returns an RCU-protected pointer".
> 
> Yeah, that seems reasonable in general, but we already have iterator
> APIs (bpf_iter_task_{new,next,destroy}()) that collectively require
> RCU CS to be open throughout the three calls. That's why I just
> cleaned up the internal logic for KF_RCU_PROTECTED and made KF_RET_RCU
> as what you're suggesting (i.e., fold KF_RCU_PROTECTED into it), which
> I assume will be most useful for the majority of kfuncs that are not
> iterators.

Right, my suggestion was to fold the KF_RET_RCU semantics into
KF_RCU_PROTECTED, even if the kfunc doesn't return anything (assuming it's
possible). Then annotate both iterators and kfuncs that require RCU as
KF_RCU_PROTECTED. This should handle both, right?

In any case, I don't have a strong opinion on that and I'm also happy with
KF_RCU_RET and KF_RCU_PROTECTED if it gives more flexibility.

Thanks,
-Andrea