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

From: Alan Maguire <alan.maguire@oracle.com>
To: David Vernet <void@manifault.com>, Jonathan Corbet <corbet@lwn.net>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	bpf@vger.kernel.org, ast@kernel.org, andrii@kernel.org,
	martin.lau@linux.dev, song@kernel.org, yhs@meta.com,
	john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
	haoluo@google.com, jolsa@kernel.org,
	linux-kernel@vger.kernel.org, kernel-team@meta.com,
	memxor@gmail.com, hch@infradead.org
Subject: Re: [PATCH bpf-next v2 3/3] bpf: Use BPF_KFUNC macro at all kfunc definitions
Date: Tue, 31 Jan 2023 15:15:25 +0000	[thread overview]
Message-ID: <7cecda8c-9499-4fc1-784c-4e6174122a1f@oracle.com> (raw)
In-Reply-To: <Y9AFT4pTydKh+PD3@maniforge.lan>

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@manifault.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.

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.
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
> 