From: Alan Maguire <alan.maguire@oracle.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Masahiro Yamada <masahiroy@kernel.org>
Cc: andrii@kernel.org, jolsa@kernel.org, acme@redhat.com,
eddyz87@gmail.com, ast@kernel.org, daniel@iogearbox.net,
martin.lau@linux.dev, song@kernel.org, yonghong.song@linux.dev,
john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
haoluo@google.com, bpf@vger.kernel.org,
linux-kbuild@vger.kernel.org
Subject: Re: [PATCH v2 bpf-next] kbuild,bpf: switch to using --btf_features for pahole v1.26 and later
Date: Tue, 14 May 2024 17:29:19 +0100 [thread overview]
Message-ID: <9a279f45-86ce-4ce0-9828-ba9cdd39c0ee@oracle.com> (raw)
In-Reply-To: <CAEf4BzZrak8+F+b-OXJOppws=88RNi_mpiSP=Z0dD=QJZKPF4A@mail.gmail.com>
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
>
prev parent reply other threads:[~2024-05-14 16:29 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240507135514.490467-1-alan.maguire@oracle.com>
[not found] ` <CAEf4BzbWANm+Bf63hcFAB3Tn51tOeBLhyabV3NNz8tjaMnThjg@mail.gmail.com>
2024-05-09 8:18 ` [PATCH v2 bpf-next] kbuild,bpf: switch to using --btf_features for pahole v1.26 and later Alan Maguire
2024-05-09 22:01 ` Andrii Nakryiko
2024-05-10 6:29 ` Masahiro Yamada
2024-05-10 21:44 ` Andrii Nakryiko
2024-05-11 9:01 ` Masahiro Yamada
2024-05-14 15:53 ` Andrii Nakryiko
2024-05-14 16:29 ` Alan Maguire [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=9a279f45-86ce-4ce0-9828-ba9cdd39c0ee@oracle.com \
--to=alan.maguire@oracle.com \
--cc=acme@redhat.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=masahiroy@kernel.org \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).