Re: [PATCH bpf-next v2 3/3] bpf: Use BPF_KFUNC macro at all kfunc definitions

[Alexei Starovoitov]

On Tue, Jan 31, 2023 at 7:44 AM David Vernet <void@xxxxxxxxxxxxx> wrote:
>
> On Tue, Jan 31, 2023 at 03:15:25PM +0000, Alan Maguire wrote:
> > On 24/01/2023 16:20, David Vernet wrote:
> > > On Tue, Jan 24, 2023 at 07:50:31AM -0700, Jonathan Corbet wrote:
> > >> David Vernet <void@xxxxxxxxxxxxx> writes:
> > >>
> > >>> I was perhaps a bit naive to think we could just throw a __bpf_kfunc
> > >>> macro onto the function signatures and call it a day :-) I think it's
> > >>> probably best to table this for now, and either I or someone else can
> > >>> come back to it when we have bandwidth to solve the problem more
> > >>> appropriately.
> > >>
> > >> Now I feel bad ... I was just tossing out a thought, not wanting to
> > >> bikeshed this work into oblivion.  If what you have solves a real
> > >
> > > No apologies necessary. I don't think this qualifies as bikeshedding.
> > > IMO folks are raising legitimate UX concerns, which is important and
> > > worth getting right.
> > >
> > >> problem and is the best that can be done now, perhaps it should just go
> > >> in and a "more appropriate" solution can be adopted later, should
> > >> somebody manage to come up with it?
> > >
> > > That would be my preference, but I also understand folks' sentiment of
> > > wanting to keep out what they feel like is odd syntax, as Christoph said
> > > in [0], and Daniel alluded to earlier in this thread.
> > >
> > > [0]: https://lore.kernel.org/all/Y8+FeH7rz8jDTubt@xxxxxxxxxxxxx/
> > >
> > > I tested on an LTO build and wrapper kfuncs (with external linkage) were
> > > not being stripped despite not being called from anywhere else in the
> > > kernel, so for now I _think_ it's safe to call this patch set more of a
> > > cleanup / future-proofing than solving an immediate and pressing problem
> > > (as long as anyone adding kfuncs carefully follows the directions in
> > > [1]). In other words, I think we have some time to do this the right way
> > > without paying too much of a cost later. If we set up the UX correctly,
> > > just adding an EXPORT_SYMBOL_KFUNC call (or something to that effect,
> > > including just using BTF_ID_FLAGS) should be minimal effort even if
> > > there are a lot more kfuncs by then.
> > >
> > > [1]: https://docs.kernel.org/bpf/kfuncs.html
> > >
> > > If it turns out that we start to observe problems in LTO builds without
> > > specifying __used and/or noinline, or if folks are repeatedly making
> > > mistakes when adding kfuncs (by e.g. not giving wrapper kfuncs external
> > > linkage) then I think it would be a stronger case to get this in now and
> > > fix it up later.
> > >
> >
> > hi David,
> >
> > I think I may have stumbled upon such a case. We're working on improving
> > the relationship between the generated BPF Type Format (BTF) info
> > for the kernel and the actual function signatures, doing things like
> > spotting optimized-out parameters and not including such functions
> > in the final BTF since tracing such functions violates user expectations.
> > The changes also remove functions with inconsistent prototypes (same
> > name, different function prototype).
> >
> > As part of that work [1], I ran into an issue with kfuncs. Because some of these
> > functions have minimal definitions, the compiler tries to be clever and as
> > a result parameters are not represented in DWARF. As a consequence of this,
> > we do not generate a BTF representation for the kfunc (since DWARF is telling
> > us the function has optimized-out parameters), and so then don't have BTF ids
> > for the associated kfunc, which is then not usable. The issue of trace accuracy
> > is important for users, so we're hoping to land those changes in dwarves soon.

Alan,

which kfuncs suffer from missing dwarf ?
I'm assuming that issues happens only with your new pahole patches
that are trying to detect all optimized out args, right?

> Hi Alan,
>
> I see. Thanks for explaining. So it seems that maybe the issue is
> slightly more urgent than we first thought. Given that folks aren't keen
> on the BPF_KFUNC macro approach that wraps the function definition,
> maybe we can go back to the __bpf_kfunc proposal from [0] as a stopgap
> solution until we can properly support something like
> EXPORT_SYMBOL_KFUNC. Alexei -- what do you think?
>
> [0]: https://lore.kernel.org/bpf/Y7kCsjBZ%2FFrsWW%2Fe@xxxxxxxxxxxxx/T/
>
> >
> > As described in [2] adding a prefixed
> >
> > __attribute__ ((optimize("O0")))

That won't work.
This attr sort-of "works" in gcc only, but it's discouraged
and officially considered broken by gcc folks.
There are projects open and close source that use this attr,
but it's very fragile.
Also we really don't want to reduce optimizations in kfuncs.
They need to be fast.

> >
> > ...to the kfunc sorts this out, so having that attribute rolled into a prefix
> > definition like the one you've proposed would solve this in the short term.
>
> Does just using __attribute__((__used__)) work? Many of these kfuncs are
> called on hotpaths in BPF programs, so compiling them with no
> optimization is not an ideal or likely even realistic option. Not to
> mention the fact that not all kfuncs are BPF-exclusive (meaning you can
> export a normal kernel function that's called by the main kernel as a
> kfunc).

let's annotate with __used and __weak to prevent compilers optimizing
things out. I think just __used won't be enough, but __weak should do
the trick. And noinline.
There is also __attribute__((visibility("hidden"))) to experiment with.