Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> writes: > 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 :-\ Yeah, I spent a while staring at the comment and reading the code to understand the nuance between the initial and default values. I don't think I fully understood it until this v3 patch, and admittedly I still didn't have the full context of how "default" was used. One interesting point of comparison is the "-M" argument to "qemu-system-$arch". For example: $ qemu-system-x86_64 -M ? Supported machines are: microvm microvm (i386) pc Standard PC (i440FX + PIIX, 1996) (alias of pc-i440fx-9.0) pc-i440fx-9.0 Standard PC (i440FX + PIIX, 1996) (default) pc-i440fx-8.2 Standard PC (i440FX + PIIX, 1996) pc-i440fx-8.1 Standard PC (i440FX + PIIX, 1996) pc-i440fx-8.0 Standard PC (i440FX + PIIX, 1996) [...] So the default "pc" machine is simply an alias that gets updated to the most recent machine (with potential new behaviors) every release, but you can always select a specific machine that you care about. Maybe it would make sense if there were versioned defaults, so that "default" always picks whatever is relevant to the most recent upstream kernel, but you could also select the default as of an older pahole release. That does sound like plenty of complexity added to an already somewhat confusing system, so I'm not sold on it. The flexibility for adjusting to new kernel defaults is appealing though. >> >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? One concern with configuration files is that (at least on my system) they tend to sit around and get forgotten, unless they're super well known configs like ~/.bashrc. So at some point, I could see myself setting a pahole config and then 6 months later wondering why pahole behaves differently on two different systems. Env vars are easy to set permanently if you want, but are also more visible and centralized with your other configurations, so they're my preference. Thanks, Stephen
