| Message ID | 20231013153359.88274-1-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pahole, btf_encoder: support --btf_features | expand |
On Fri, Oct 13, 2023 at 04:33:54PM +0100, Alan Maguire wrote: > Currently, the kernel uses pahole version checking as the way to > determine which BTF encoding features to request from pahole. This > means that such features have to be tied to a specific version and > as new features are added, additional clauses in scripts/pahole-flags.sh > have to be added; for example > > if [ "${pahole_ver}" -ge "125" ]; then > extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > fi > > To better future-proof this process, this series introduces a > single "btf_features" parameter that uses a comma-separated list > of encoding options. This is helpful because > > - the semantics are simpler for the user; the list comprises the set of > BTF features asked for, rather than having to specify a combination of > --skip_encoding_btf_feature and --btf_gen_feature options; and > - any version of pahole that supports --btf_features can accept the > option list; unknown options are silently ignored. As a result, there > would be no need to add additional version clauses beyond > > if [ "${pahole_ver}" -ge "126" ]; then > extra_pahole_opt="-j --lang_exclude=rust > --btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized,consistent" > fi > > Newly-supported features would simply be appended to the btf_features > list, and these would have impact on BTF encoding only if the features > were supported by pahole. This means pahole will not require a version > bump when new BTF features are added, and should ease the burden of > coordinating such changes between bpf-next and dwarves. > > Patches 1 and 2 are preparatory work, while patch 3 adds the > --btf_features support. Patch 4 provides a means of querying > the supported feature set since --btf_features will not error > out when it encounters unrecognized features (this ensures > an older pahole without a requested feature will not dump warnings > in the build log for kernel/module BTF generation). Patch 5 > adds --btf_features_strict, which is identical to --btf_features > aside from the fact it will fail if an unrecognized feature is used. > > See [1] for more background on this topic. > > Changes since RFC [2]: > > - ensure features are disabled unless requested; use "default" field in > "struct btf_features" to specify the conf_load default value which > corresponds to the feature being disabled. For > conf_load->btf_gen_floats for example, the default value is false, > while for conf_load->skip_encoding_btf_type_tags the default is > true; in both cases the intent is to _not_ encode the associated > feature by default. However if the user specifies "float" or > "type_tag" in --btf_features, the default conf_load value is negated, > resulting in a BTF encoding that contains floats and type tags > (Eduard, patch 3) > - clarify feature default/setting behaviour and how it only applies > when --btf_features is used (Eduard, patch 3) > - ensure we do not run off the end of the feature_list[] array > (Eduard, patch 3) > - rather than having each struct btf_feature record the offset in the > conf_load structure of the boolean (requiring us to later do pointer > math to update it), record the pointers to the boolean conf_load > values associated with each feature (Jiri, patch 3) > - allow for multiple specifications of --btf_features, enabling the > union of all features specified (Andrii, patch 3) > - rename function-related optimized/consistent to optimized_func and > consistent_func in recognition of the fact they are function-specific > (Andrii, patch 3) > - add a strict version of --btf_features, --btf_features_strict that > will error out if an unrecognized feature is used (Andrii, patch 5) > > [1] https://lore.kernel.org/bpf/CAEf4Bzaz1UqqxuZ7Q+KQee-HLyY1nwhAurBE2n9YTWchqoYLbg@mail.gmail.com/ > [2] https://lore.kernel.org/bpf/20231011091732.93254-1-alan.maguire@oracle.com/ > Acked-by: Jiri Olsa <jolsa@kernel.org> jirka > Alan Maguire (5): > btf_encoder, pahole: move btf encoding options into conf_load > dwarves: move ARRAY_SIZE() to dwarves.h > pahole: add --btf_features support > pahole: add --supported_btf_features > pahole: add --btf_features_strict to reject unknown BTF features > > btf_encoder.c | 8 +-- > btf_encoder.h | 2 +- > dwarves.c | 16 ----- > dwarves.h | 19 ++++++ > man-pages/pahole.1 | 32 +++++++++ > pahole.c | 167 +++++++++++++++++++++++++++++++++++++++++---- > 6 files changed, 210 insertions(+), 34 deletions(-) > > -- > 2.31.1 >
On Fri, 2023-10-13 at 16:33 +0100, Alan Maguire wrote: > Currently, the kernel uses pahole version checking as the way to > determine which BTF encoding features to request from pahole. This > means that such features have to be tied to a specific version and > as new features are added, additional clauses in scripts/pahole-flags.sh > have to be added; for example > > if [ "${pahole_ver}" -ge "125" ]; then > extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > fi > > To better future-proof this process, this series introduces a > single "btf_features" parameter that uses a comma-separated list > of encoding options. This is helpful because > > - the semantics are simpler for the user; the list comprises the set of > BTF features asked for, rather than having to specify a combination of > --skip_encoding_btf_feature and --btf_gen_feature options; and > - any version of pahole that supports --btf_features can accept the > option list; unknown options are silently ignored. As a result, there > would be no need to add additional version clauses beyond > > if [ "${pahole_ver}" -ge "126" ]; then > extra_pahole_opt="-j --lang_exclude=rust > --btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized,consistent" > fi Nitpick, could you please update the line above as below? --btf_features_strict=encode_force,var,float,decl_tag,type_tag,enum64,optimized_func,consistent_func
On Tue, Oct 17, 2023 at 5:57 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Fri, 2023-10-13 at 16:33 +0100, Alan Maguire wrote: > > Currently, the kernel uses pahole version checking as the way to > > determine which BTF encoding features to request from pahole. This > > means that such features have to be tied to a specific version and > > as new features are added, additional clauses in scripts/pahole-flags.sh > > have to be added; for example > > > > if [ "${pahole_ver}" -ge "125" ]; then > > extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > fi > > > > To better future-proof this process, this series introduces a > > single "btf_features" parameter that uses a comma-separated list > > of encoding options. This is helpful because > > > > - the semantics are simpler for the user; the list comprises the set of > > BTF features asked for, rather than having to specify a combination of > > --skip_encoding_btf_feature and --btf_gen_feature options; and > > - any version of pahole that supports --btf_features can accept the > > option list; unknown options are silently ignored. As a result, there > > would be no need to add additional version clauses beyond > > > > if [ "${pahole_ver}" -ge "126" ]; then > > extra_pahole_opt="-j --lang_exclude=rust > > --btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized,consistent" > > fi > > Nitpick, could you please update the line above as below? > > --btf_features_strict=encode_force,var,float,decl_tag,type_tag,enum64,optimized_func,consistent_func We do not want "strict" behavior for the kernel build script. I'd say we will want to use a multi arg variant as it's more composable: --btf_features=encode_force \ --btf_feature=var \ and so on. Either way, this patch set overall looks good to me: Acked-by: Andrii Nakryiko <andrii@kernel.org>
On Tue, 2023-10-17 at 11:53 -0700, Andrii Nakryiko wrote: > > > if [ "${pahole_ver}" -ge "126" ]; then > > > extra_pahole_opt="-j --lang_exclude=rust > > > --btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized,consistent" > > > fi > > > > Nitpick, could you please update the line above as below? > > > > --btf_features_strict=encode_force,var,float,decl_tag,type_tag,enum64,optimized_func,consistent_func > > We do not want "strict" behavior for the kernel build script. Oh, sorry, my comment was about '_func' suffixes, it should have been --btf_features, of course.
Currently, the kernel uses pahole version checking as the way to determine which BTF encoding features to request from pahole. This means that such features have to be tied to a specific version and as new features are added, additional clauses in scripts/pahole-flags.sh have to be added; for example if [ "${pahole_ver}" -ge "125" ]; then extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" fi To better future-proof this process, this series introduces a single "btf_features" parameter that uses a comma-separated list of encoding options. This is helpful because - the semantics are simpler for the user; the list comprises the set of BTF features asked for, rather than having to specify a combination of --skip_encoding_btf_feature and --btf_gen_feature options; and - any version of pahole that supports --btf_features can accept the option list; unknown options are silently ignored. As a result, there would be no need to add additional version clauses beyond if [ "${pahole_ver}" -ge "126" ]; then extra_pahole_opt="-j --lang_exclude=rust --btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized,consistent" fi Newly-supported features would simply be appended to the btf_features list, and these would have impact on BTF encoding only if the features were supported by pahole. This means pahole will not require a version bump when new BTF features are added, and should ease the burden of coordinating such changes between bpf-next and dwarves. Patches 1 and 2 are preparatory work, while patch 3 adds the --btf_features support. Patch 4 provides a means of querying the supported feature set since --btf_features will not error out when it encounters unrecognized features (this ensures an older pahole without a requested feature will not dump warnings in the build log for kernel/module BTF generation). Patch 5 adds --btf_features_strict, which is identical to --btf_features aside from the fact it will fail if an unrecognized feature is used. See [1] for more background on this topic. Changes since RFC [2]: - ensure features are disabled unless requested; use "default" field in "struct btf_features" to specify the conf_load default value which corresponds to the feature being disabled. For conf_load->btf_gen_floats for example, the default value is false, while for conf_load->skip_encoding_btf_type_tags the default is true; in both cases the intent is to _not_ encode the associated feature by default. However if the user specifies "float" or "type_tag" in --btf_features, the default conf_load value is negated, resulting in a BTF encoding that contains floats and type tags (Eduard, patch 3) - clarify feature default/setting behaviour and how it only applies when --btf_features is used (Eduard, patch 3) - ensure we do not run off the end of the feature_list[] array (Eduard, patch 3) - rather than having each struct btf_feature record the offset in the conf_load structure of the boolean (requiring us to later do pointer math to update it), record the pointers to the boolean conf_load values associated with each feature (Jiri, patch 3) - allow for multiple specifications of --btf_features, enabling the union of all features specified (Andrii, patch 3) - rename function-related optimized/consistent to optimized_func and consistent_func in recognition of the fact they are function-specific (Andrii, patch 3) - add a strict version of --btf_features, --btf_features_strict that will error out if an unrecognized feature is used (Andrii, patch 5) [1] https://lore.kernel.org/bpf/CAEf4Bzaz1UqqxuZ7Q+KQee-HLyY1nwhAurBE2n9YTWchqoYLbg@mail.gmail.com/ [2] https://lore.kernel.org/bpf/20231011091732.93254-1-alan.maguire@oracle.com/ Alan Maguire (5): btf_encoder, pahole: move btf encoding options into conf_load dwarves: move ARRAY_SIZE() to dwarves.h pahole: add --btf_features support pahole: add --supported_btf_features pahole: add --btf_features_strict to reject unknown BTF features btf_encoder.c | 8 +-- btf_encoder.h | 2 +- dwarves.c | 16 ----- dwarves.h | 19 ++++++ man-pages/pahole.1 | 32 +++++++++ pahole.c | 167 +++++++++++++++++++++++++++++++++++++++++---- 6 files changed, 210 insertions(+), 34 deletions(-)