Re: [PATCH dwarves v3 5/5] pahole: add global_var BTF feature

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Thu, Oct 03, 2024 at 04:21:07PM +0100, Alan Maguire wrote:
> On 03/10/2024 15:53, Arnaldo Carvalho de Melo wrote:
> > On Thu, Oct 03, 2024 at 03:40:35PM +0100, Alan Maguire wrote:
> >> On 03/10/2024 00:52, Stephen Brennan wrote:
> >>> So far, pahole has only encoded type information for percpu variables.
> >>> However, there are several reasons why type information for all global
> >>> variables would be useful in the kernel:
> > 
> >>> 1. Runtime kernel debuggers like drgn could use the BTF to introspect
> >>> kernel data structures without needing to install heavyweight DWARF.
> > 
> >>> 2. BPF programs using the "__ksym" annotation could declare the
> >>> variables using the correct type, rather than "void".
> > 
> >>> It makes sense to introduce a feature for this in pahole so that these
> >>> capabilities can be explored in the kernel. The feature is non-default:
> >>> when using "--btf-features=default", it is disabled. It must be
> >>> explicitly requested, e.g. with "--btf-features=+global_var".
> >  
> >> I'm not totally sure switching global_var to a non-default feature is
> >> the right answer here.
> >  
> >> The --btf_features "default" set of options are meant to closely mirror
> >> the upstream kernel options we enable when doing BTF encoding. However,
> >> in scripts/Makefile.btf we don't use "default"; we explicitly call out
> >> the set of features we want. We can't just use "default" in that context
> >> since the meaning of "default" varies based upon whatever version of
> >> pahole you have.
> >  
> >> So "default" is simply a convenient shorthand for pahole testing which
> >> corresponds to "give me the set of features that upstream kernels use".
> >> It could have a better name that reflects that more clearly I suppose.
> >  
> >> When we do switch this on in-kernel, we'll add the explicit "global_var"
> >> to the list of features in scripts/Makefile.btf.
> >  
> >> So with all this said, do we make global_vars a default or non-default
> >> feature? It would seem to make sense to specify non-default, since it is
> >> not switched on for the kernel yet, but looking ahead, what if the 1.28
> >> pahole release is used to build vmlinux BTF and we add global_var to the
> >> list of features? In such a case, our "default" set of values would be
> >> out of step with the kernel. So it's not a huge deal, but I would
> >> consider keeping this a default feature to facilitate testing; this
> >> won't change what the kernel does, but it makes testing with full
> >> variable generation easier (I can just do "--btf_features=default").
> > 
> > This "default" really is confusing, as you spelled out above :-\ When to
> > add something to it so that it reflects what the kernel has is tricky,
> > perhaps we should instead have a ~/.config/pahole file where developers
> > can add BTF features to add to --btf_features=default in the period
> > where something new was _really_ added to the kernel and before the next
> > version when it _have been added to the kernel set of BTF features_ thus
> > should be set into stone in the pahole sources?
 
> it's a nice idea; I suppose once we have more automated tests, this will
> be less of an issue too. I'm looking at adding a BTF variable test
> shortly, would be good to have coverage there too, especially since
> we're growing the amount of info we encode in this area.

Sure thing, the more tests, the better!
 
> > So I think we should do as Stephen did, keep it out of
> > --btf_features=default, as it is not yet in the kernel set of options,
> > and have the config file, starting with being able to set those
> > features, i.e. we would have:

> > $ cat ~/.config/pahole
> > [btf_encoder]
> > 	btf_features=+global_var

> > wdyt?
 
> I think that makes perfect sense, great idea!

I was looking for a library to do that to avoid "stealing" the
perf-config code, but perhaps we should use an env var for that?

PAHOLE_BTF_FEATURES='+global_var'

To keep things simple?

- Arnaldo