Message-ID: <Y9k3bz5h8VDqDoZw@maniforge>
Date:   Tue, 31 Jan 2023 09:44:47 -0600
From:   David Vernet <void@...ifault.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Jonathan Corbet <corbet@....net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        bpf@...r.kernel.org, ast@...nel.org, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...a.com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, linux-kernel@...r.kernel.org,
        kernel-team@...a.com, memxor@...il.com, hch@...radead.org
Subject: Re: [PATCH bpf-next v2 3/3] bpf: Use BPF_KFUNC macro at all kfunc
 definitions

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@...ifault.com> 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@infradead.org/
> > 
> > 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.

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@maniforge.lan/T/

>  
> As described in [2] adding a prefixed
> 
> __attribute__ ((optimize("O0"))) 
> 
> ...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).

> There may be a better way to solve the problem I've run into, but I can't
> see an easy solution right now.
> 
> Would the above be feasible do you think? Thanks!
> 
> Alan
> 
> [1] https://lore.kernel.org/bpf/1675088985-20300-1-git-send-email-alan.maguire@oracle.com/
> [2] https://lore.kernel.org/bpf/fe5d42d1-faad-d05e-99ad-1c2c04776950@oracle.com/
> 
>  

Thanks,
David

Powered by blists - more mailing lists