From: Eduard Zingerman <eddyz87@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>,
acme@kernel.org, andrii.nakryiko@gmail.com
Cc: jolsa@kernel.org, ast@kernel.org, daniel@iogearbox.net,
martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
haoluo@google.com, mykolal@fb.com, bpf@vger.kernel.org
Subject: Re: [PATCH v3 dwarves 0/5] pahole, btf_encoder: support --btf_features
Date: Thu, 19 Oct 2023 14:07:00 +0300 [thread overview]
Message-ID: <ca6ca9a23f4ad8e679e873940beceef4c7c172a7.camel@gmail.com> (raw)
In-Reply-To: <20231018122926.735416-1-alan.maguire@oracle.com>
On Wed, 2023-10-18 at 13:29 +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_func,consistent_func"
> 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 v2 [2]:
> - added acks from Andrii and Jiri (patches 1-5)
> - merged suggestions from Eduard which simplify and clean up code
> considerably; these changes fix issues with --btf_features_strict
> while providing better diagnostic output when an unknown feature
> is encountered (specifying the unknown feature if in verbose
> or strict modes). Added Suggested-bys from Eduard for the
> relevant patches (Eduard, patches 3,5)
>
> Changes since RFC [3]:
>
> - 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/20231013153359.88274-1-alan.maguire@oracle.com/
> [3] https://lore.kernel.org/bpf/20231011091732.93254-1-alan.maguire@oracle.com/
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Hi Alan,
thank you for accommodating my changes, I've tested this patch-set and
everything seems to work fine.
I left one nitpick for patch #3 feel free to ignore it if you don't agree.
Thanks,
Eduard
prev parent reply other threads:[~2023-10-19 11:07 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 12:29 [PATCH v3 dwarves 0/5] pahole, btf_encoder: support --btf_features Alan Maguire
2023-10-18 12:29 ` [PATCH v3 dwarves 1/5] btf_encoder, pahole: move btf encoding options into conf_load Alan Maguire
2023-10-18 12:29 ` [PATCH v3 dwarves 2/5] dwarves: move ARRAY_SIZE() to dwarves.h Alan Maguire
2023-10-18 12:29 ` [PATCH v3 dwarves 3/5] pahole: add --btf_features support Alan Maguire
2023-10-19 11:07 ` Eduard Zingerman
2023-10-19 21:24 ` Alan Maguire
2023-10-18 12:29 ` [PATCH v3 dwarves 4/5] pahole: add --supported_btf_features Alan Maguire
2023-10-18 12:29 ` [PATCH v3 dwarves 5/5] pahole: add --btf_features_strict to reject unknown BTF features Alan Maguire
2023-10-19 11:07 ` Eduard Zingerman [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ca6ca9a23f4ad8e679e873940beceef4c7c172a7.camel@gmail.com \
--to=eddyz87@gmail.com \
--cc=acme@kernel.org \
--cc=alan.maguire@oracle.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=martin.lau@linux.dev \
--cc=mykolal@fb.com \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).