Re: [PATCH bpf-next v1 2/5] cgroup: bpf: add cgroup_rstat_updated() and cgroup_rstat_flush() kfuncs

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Fri, May 20, 2022 at 02:43:03PM IST, Yosry Ahmed wrote:
> On Fri, May 20, 2022 at 12:24 AM Tejun Heo <tj@kernel.org> wrote:
> >
> > On Fri, May 20, 2022 at 01:21:30AM +0000, Yosry Ahmed wrote:
> > > Add cgroup_rstat_updated() and cgroup_rstat_flush() kfuncs to bpf
> > > tracing programs. bpf programs that make use of rstat can use these
> > > functions to inform rstat when they update stats for a cgroup, and when
> > > they need to flush the stats.
> > >
> > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> >
> > Do patch 1 and 2 need to be separate? Also, can you explain and comment why
> > it's __weak?
>
> I will squash them in the next version.
>
> As for the declaration, I took the __weak annotation from Alexei's
> reply to the previous version. I thought it had something to do with
> how fentry progs attach to functions with BTF and all.
> When I try the same code with a static noinline declaration instead,
> fentry attachment fails to find the BTF type ID of bpf_rstat_flush.
> When I try it with just noinline (without __weak), the fentry program
> attaches, but is never invoked. I tried looking at the attach code but
> I couldn't figure out why this happens.
>

With static noinline, the compiler will optimize away the function. With global
noinline, it can still optimize away the call site, but will keep the function
definition, so attach works. Therefore __weak is needed to ensure call is still
emitted. With GCC __attribute__((noipa)) might have been more appropritate, but
LLVM doesn't support it, so __weak is the next best thing supported by both with
the same side effect.

> In retrospect, I should have given this more thought. It would be
> great if Alexei could shed some light on this.
>
> >
> > Thanks.
>
>
> >
> > --
> > tejun

--
Kartikeya