Re: [PATCH v2 bpf-next] kbuild,bpf: switch to using --btf_features for pahole v1.26 and later

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, May 11, 2024 at 6:45 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, May 9, 2024 at 11:30 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > On Fri, May 10, 2024 at 7:01 AM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Thu, May 9, 2024 at 1:20 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> > > >
> > > > On 07/05/2024 17:48, Andrii Nakryiko wrote:
> > > > > On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> > > > >>
> > > > >> The btf_features list can be used for pahole v1.26 and later -
> > > > >> it is useful because if a feature is not yet implemented it will
> > > > >> not exit with a failure message.  This will allow us to add feature
> > > > >> requests to the pahole options without having to check pahole versions
> > > > >> in future; if the version of pahole supports the feature it will be
> > > > >> added.
> > > > >>
> > > > >> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> > > > >> Tested-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > > > >> ---
> > > > >>  scripts/Makefile.btf | 15 +++++++++++++--
> > > > >>  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > >>
> > > > >> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> > > > >> index 82377e470aed..2d6e5ed9081e 100644
> > > > >> --- a/scripts/Makefile.btf
> > > > >> +++ b/scripts/Makefile.btf
> > > > >> @@ -3,6 +3,8 @@
> > > > >>  pahole-ver := $(CONFIG_PAHOLE_VERSION)
> > > > >>  pahole-flags-y :=
> > > > >>
> > > > >> +ifeq ($(call test-le, $(pahole-ver), 125),y)
> > > > >> +
> > > > >>  # pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
> > > > >>  ifeq ($(call test-le, $(pahole-ver), 121),y)
> > > > >>  pahole-flags-$(call test-ge, $(pahole-ver), 118)       += --skip_encoding_btf_vars
> > > > >> @@ -12,8 +14,17 @@ pahole-flags-$(call test-ge, $(pahole-ver), 121)     += --btf_gen_floats
> > > > >>
> > > > >>  pahole-flags-$(call test-ge, $(pahole-ver), 122)       += -j
> > > > >>
> > > > >> -pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         += --lang_exclude=rust
> > > > >> +ifeq ($(pahole-ver), 125)
> > > > >
> > > > > it's a bit of a scope creep, but isn't it strange that we don't have
> > > > > test-eq and have to work-around that with more verbose constructs?
> > > >
> > > > Looking at the history, I _think_ the concern that motivated the numeric
> > > > comparison constructs was the shell process fork required for numeric
> > > > comparisons. In the equality case, ifeq would work for both strings and
> > > > numeric values. Adding a test-eq (in a similar form to test-ge) would
> > > > require a fallback to shell expansion for older Make without intcmp, and
> > > > that would be slower than using ifeq, if less verbose.
> > > >
> > > > > Let's do a good service to the community and add test-eq (and maybe
> > > > > test-ne while at it, don't know, up to Masahiro)?
> > > > >
> > > >
> > > > Sure, I'm happy to do this if kbuild folks agree. I've cc'ed them; I
> > > > neglected to do this in the original patch, apologies about that.
> > > >
> > >
> > > Ok, let's see if Masahiro would like this improvement or not. For now
> > > this patch gets us into a nicer place where there are legacy parts and
> > > a better --btf_features setup completely separate, so I applied the
> > > patch as is to bpf-next. If we decide to do test-eq, we can improve
> > > this further separately. Thanks!
> >
> >
> > That is a noise change.
> > You did not need to modify the line in the first place.
> >
>
> Not sure which specific line you are referring to. But the idea here
> is that starting from pahole 1.26 we want to stop to set those
> "legacy" arguments (like --skip_encoding_btf_vars, --btf_gen_floats)
> and *only* use more usable and forward-compatible --btf_features.
>
> >
> > The previous
> >
> >   pahole-flags-$(call test-ge, $(pahole-ver), 125)
> >
> > works as-is.


You did not not need to change

  pahole-flags-$(call test-ge, $(pahole-ver), 125) += ...


to


  ifeq ($(pahole-ver), 125)
  pahole-flags-y += ...
  endif



Please note it exists in

  ifeq ($(call test-le, $(pahole-ver), 125),y)
     ...
  else





if (pahole_ver <= 125) {
      do_something();
      if (pahole_ver >= 125)
             do_other();
}


  and


if (pahole_ver <= 125) {
      do_something();
      if (pahole_ver == 125)
            do_other();
}


are equivalent, don't they?



The former is more intuitive because pahole 1.25+ supports
--skip_encoding_btf_inconsistent_proto --btf_gen_optimized



I attached a simpler and more correct patch.










-- 
Best Regards
Masahiro Yamada
diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
index 82377e470aed..bca8a8f26ea4 100644
--- a/scripts/Makefile.btf
+++ b/scripts/Makefile.btf
@@ -3,6 +3,8 @@
 pahole-ver := $(CONFIG_PAHOLE_VERSION)
 pahole-flags-y :=
 
+ifeq ($(call test-le, $(pahole-ver), 125),y)
+
 # pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
 ifeq ($(call test-le, $(pahole-ver), 121),y)
 pahole-flags-$(call test-ge, $(pahole-ver), 118)	+= --skip_encoding_btf_vars
@@ -12,8 +14,15 @@ pahole-flags-$(call test-ge, $(pahole-ver), 121)	+= --btf_gen_floats
 
 pahole-flags-$(call test-ge, $(pahole-ver), 122)	+= -j
 
-pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)		+= --lang_exclude=rust
-
 pahole-flags-$(call test-ge, $(pahole-ver), 125)	+= --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
 
+else
+
+# Switch to using --btf_features for v1.26 and later.
+pahole-flags-$(call test-ge, $(pahole-ver), 126)  = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func
+
+endif
+
+pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)		+= --lang_exclude=rust
+
 export PAHOLE_FLAGS := $(pahole-flags-y)

[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux