diff mbox series

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

Message ID 20240507135514.490467-1-alan.maguire@oracle.com (mailing list archive)
State Accepted
Commit fcd1ed89a0439c45e1336bd9649485c44b7597c7
Delegated to: BPF
Headers show
Series [v2,bpf-next] kbuild,bpf: switch to using --btf_features for pahole v1.26 and later | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Alan Maguire May 7, 2024, 1:55 p.m. UTC 
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@oracle.com>
Tested-by: Eduard Zingerman <eddyz87@gmail.com>
---
 scripts/Makefile.btf | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko May 7, 2024, 4:48 p.m. UTC | #1 
On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  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?
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)?

Overall the change looks OK to me, so if people are opposed to adding
test-eq, I'm fine with it as well:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> +pahole-flags-y += --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
> +endif
> +
> +else
>
> -pahole-flags-$(call test-ge, $(pahole-ver), 125)       += --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
> +# 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)
> --
> 2.39.3
>
Alan Maguire May 9, 2024, 8:18 a.m. UTC | #2 
On 07/05/2024 17:48, Andrii Nakryiko wrote:
> On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
>> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
>> ---
>>  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.

Thanks!

Alan

> Overall the change looks OK to me, so if people are opposed to adding
> test-eq, I'm fine with it as well:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
>> +pahole-flags-y += --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
>> +endif
>> +
>> +else
>>
>> -pahole-flags-$(call test-ge, $(pahole-ver), 125)       += --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
>> +# 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)
>> --
>> 2.39.3
>>
Andrii Nakryiko May 9, 2024, 10:01 p.m. UTC | #3 
On Thu, May 9, 2024 at 1:20 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 07/05/2024 17:48, Andrii Nakryiko wrote:
> > On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> >> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
> >> ---
> >>  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!


> Thanks!
>
> Alan
>
> > Overall the change looks OK to me, so if people are opposed to adding
> > test-eq, I'm fine with it as well:
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> >> +pahole-flags-y += --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
> >> +endif
> >> +
> >> +else
> >>
> >> -pahole-flags-$(call test-ge, $(pahole-ver), 125)       += --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
> >> +# 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)
> >> --
> >> 2.39.3
> >>
patchwork-bot+netdevbpf@kernel.org May 9, 2024, 10:10 p.m. UTC | #4 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Tue,  7 May 2024 14:55:14 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next] kbuild,bpf: switch to using --btf_features for pahole v1.26 and later
    https://git.kernel.org/bpf/bpf-next/c/fcd1ed89a043

You are awesome, thank you!
Masahiro Yamada May 10, 2024, 6:29 a.m. UTC | #5 
On Fri, May 10, 2024 at 7:01 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, May 9, 2024 at 1:20 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On 07/05/2024 17:48, Andrii Nakryiko wrote:
> > > On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > >> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
> > >> ---
> > >>  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.


The previous

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

works as-is.




--
Best Regards
Masahiro Yamada
Andrii Nakryiko May 10, 2024, 9:44 p.m. UTC | #6 
On Thu, May 9, 2024 at 11:30 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Fri, May 10, 2024 at 7:01 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, May 9, 2024 at 1:20 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > >
> > > On 07/05/2024 17:48, Andrii Nakryiko wrote:
> > > > On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > > >> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
> > > >> ---
> > > >>  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.
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
Masahiro Yamada May 11, 2024, 9:01 a.m. UTC | #7 
On Sat, May 11, 2024 at 6:45 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, May 9, 2024 at 11:30 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Fri, May 10, 2024 at 7:01 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Thu, May 9, 2024 at 1:20 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > >
> > > > On 07/05/2024 17:48, Andrii Nakryiko wrote:
> > > > > On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > > > >> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
> > > > >> ---
> > > > >>  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.
Andrii Nakryiko May 14, 2024, 3:53 p.m. UTC | #8 
On Sat, May 11, 2024 at 3:01 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sat, May 11, 2024 at 6:45 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, May 9, 2024 at 11:30 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Fri, May 10, 2024 at 7:01 AM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Thu, May 9, 2024 at 1:20 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > >
> > > > > On 07/05/2024 17:48, Andrii Nakryiko wrote:
> > > > > > On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > > > > >> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
> > > > > >> ---
> > > > > >>  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

The point here is to not specify these "legacy" arguments starting
from pahole v1.26, and the patch makes it more obvious, IMO.

But I don't mind adding (test-ge,125) check back, Alan, please send a
follow up patch.

>
>
>
> I attached a simpler and more correct patch.

It's not more correct, because this patch didn't break anything. It's
just as correct.

>
>
>
>
>
>
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
Alan Maguire May 14, 2024, 4:29 p.m. UTC | #9 
On 14/05/2024 16:53, Andrii Nakryiko wrote:
> On Sat, May 11, 2024 at 3:01 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>>
>> On Sat, May 11, 2024 at 6:45 AM Andrii Nakryiko
>> <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Thu, May 9, 2024 at 11:30 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>>
>>>> On Fri, May 10, 2024 at 7:01 AM Andrii Nakryiko
>>>> <andrii.nakryiko@gmail.com> wrote:
>>>>>
>>>>> On Thu, May 9, 2024 at 1:20 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>>>>
>>>>>> On 07/05/2024 17:48, Andrii Nakryiko wrote:
>>>>>>> On Tue, May 7, 2024 at 6:55 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
>>>>>>>> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
>>>>>>>> ---
>>>>>>>>  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
> 
> The point here is to not specify these "legacy" arguments starting
> from pahole v1.26, and the patch makes it more obvious, IMO.
> 
> But I don't mind adding (test-ge,125) check back, Alan, please send a
> follow up patch.
>

Done, see [1]. Thanks!

Alan

[1]
https://lore.kernel.org/bpf/20240514162716.2448265-1-alan.maguire@oracle.com/

>>
>>
>>
>> I attached a simpler and more correct patch.
> 
> It's not more correct, because this patch didn't break anything. It's
> just as correct.
> 
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> --
>> Best Regards
>> Masahiro Yamada
>
diff mbox series

Patch

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)
+pahole-flags-y	+= --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
+endif
+
+else
 
-pahole-flags-$(call test-ge, $(pahole-ver), 125)	+= --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
+# 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)