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").
And on that subject, I tested with this series, and all looks good.
vmlinux BTF grew by 1.5Mb to 6.7Mb for me on a bpf-next kernel.
Following datasecs were seen:
[156581] DATASEC '.rodata' size=7379360 vlen=5472
[156582] DATASEC '__init_rodata' size=496 vlen=3
[156583] DATASEC '__param' size=15160 vlen=375
[156584] DATASEC '__modver' size=864 vlen=12
[156585] DATASEC '.data' size=5955041 vlen=15839
[156586] DATASEC '.vvar' size=656 vlen=2
[156587] DATASEC '.data..percpu' size=229632 vlen=389
[156588] DATASEC '.init.data' size=2057888 vlen=5565
[156589] DATASEC '.x86_cpu_dev.init' size=40 vlen=5
[156590] DATASEC '.apicdrivers' size=56 vlen=7
[156591] DATASEC '.data_nosave' size=4 vlen=1
[156592] DATASEC '.bss' size=3788800 vlen=4080
[156593] DATASEC '.brk' size=196608 vlen=2
[156594] DATASEC '.init.scratch' size=4194304 vlen=1
Biggest contributors in terms of BTF size appear to be
- .data (15839 vars)
- .init.data (5565 vars)
- .rodata (5472 vars)
- .bss (4080 vars)
> Signed-off-by: Stephen Brennan <stephen.s.brennan@xxxxxxxxxx>
Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
Tested-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> btf_encoder.c | 5 +++++
> btf_encoder.h | 1 +
> dwarves.h | 1 +
> man-pages/pahole.1 | 7 +++++--
> pahole.c | 3 ++-
> 5 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 2fd1648..2730ea8 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -2348,6 +2348,8 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> encoder->encode_vars = 0;
> if (!conf_load->skip_encoding_btf_vars)
> encoder->encode_vars |= BTF_VAR_PERCPU;
> + if (conf_load->encode_btf_global_vars)
> + encoder->encode_vars |= BTF_VAR_GLOBAL;
>
> GElf_Ehdr ehdr;
>
> @@ -2400,6 +2402,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> encoder->secinfo[shndx].name = secname;
> encoder->secinfo[shndx].type = shdr.sh_type;
>
> + if (encoder->encode_vars & BTF_VAR_GLOBAL)
> + encoder->secinfo[shndx].include = true;
> +
> if (strcmp(secname, PERCPU_SECTION) == 0) {
> found_percpu = true;
> if (encoder->encode_vars & BTF_VAR_PERCPU)
> diff --git a/btf_encoder.h b/btf_encoder.h
> index 91e7947..824963b 100644
> --- a/btf_encoder.h
> +++ b/btf_encoder.h
> @@ -20,6 +20,7 @@ struct list_head;
> enum btf_var_option {
> BTF_VAR_NONE = 0,
> BTF_VAR_PERCPU = 1,
> + BTF_VAR_GLOBAL = 2,
> };
>
> struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
> diff --git a/dwarves.h b/dwarves.h
> index 0fede91..fef881f 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -92,6 +92,7 @@ struct conf_load {
> bool btf_gen_optimized;
> bool skip_encoding_btf_inconsistent_proto;
> bool skip_encoding_btf_vars;
> + bool encode_btf_global_vars;
> bool btf_gen_floats;
> bool btf_encode_force;
> bool reproducible_build;
> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> index b3e6632..7c1a69a 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -238,7 +238,9 @@ the debugging information.
>
> .TP
> .B \-\-skip_encoding_btf_vars
> -Do not encode VARs in BTF.
> +By default, VARs are encoded only for percpu variables. When specified, this
> +option prevents encoding any VARs. Note that this option can be overridden
> +by the feature "global_var".
>
> .TP
> .B \-\-skip_encoding_btf_decl_tag
> @@ -304,7 +306,7 @@ Encode BTF using the specified feature list, or specify 'default' for all standa
> encode_force Ignore invalid symbols when encoding BTF; for example
> if a symbol has an invalid name, it will be ignored
> and BTF encoding will continue.
> - var Encode variables using BTF_KIND_VAR in BTF.
> + var Encode percpu variables using BTF_KIND_VAR in BTF.
> float Encode floating-point types in BTF.
> decl_tag Encode declaration tags using BTF_KIND_DECL_TAG.
> type_tag Encode type tags using BTF_KIND_TYPE_TAG.
> @@ -329,6 +331,7 @@ Supported non-standard features (not enabled for 'default')
> the associated base BTF to support later relocation
> of split BTF with a possibly changed base, storing
> it in a .BTF.base ELF section.
> + global_var Encode all global variables using BTF_KIND_VAR in BTF.
> .fi
>
> So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values.
> diff --git a/pahole.c b/pahole.c
> index b21a7f2..9f0dc59 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1301,6 +1301,7 @@ struct btf_feature {
> BTF_DEFAULT_FEATURE(decl_tag_kfuncs, btf_decl_tag_kfuncs, false),
> BTF_NON_DEFAULT_FEATURE(reproducible_build, reproducible_build, false),
> BTF_NON_DEFAULT_FEATURE(distilled_base, btf_gen_distilled_base, false),
> + BTF_NON_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
see above, I'd suggest making this a BTF_DEFAULT_FEATURE() to make
testing easier.
> };
>
> #define BTF_MAX_FEATURE_STR 1024
> @@ -1733,7 +1734,7 @@ static const struct argp_option pahole__options[] = {
> {
> .name = "skip_encoding_btf_vars",
> .key = ARGP_skip_encoding_btf_vars,
> - .doc = "Do not encode VARs in BTF."
> + .doc = "Do not encode any VARs in BTF [if this is not specified, only percpu variables are encoded. To encode global variables too, use --encode_btf_global_vars]."
> },
> {
> .name = "btf_encode_force",
