On Thu, Oct 03, 2024 at 10:48:23AM -0700, Stephen Brennan wrote: > 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. Agreed, lets go with an env var. And this one is even "ephemeral", i.e. as we get new versions of pahole it should match the most recent set of kernel btf_features set and thus become unneeded. At some point we'll stop adding features, right? 8-) - Arnaldo
