Date:   Wed, 13 Jan 2021 14:38:27 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Nathan Chancellor <natechancellor@...il.com>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Sedat Dilek <sedat.dilek@...il.com>
Subject: Re: [PATCH] bpf: Hoise pahole version checks into Kconfig

On Mon, Jan 11, 2021 at 1:24 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Mon, Jan 11, 2021 at 12:00 PM Nathan Chancellor
> <natechancellor@...il.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 04:50:50AM +0900, Masahiro Yamada wrote:
> > > On Tue, Jan 12, 2021 at 4:34 AM Nathan Chancellor
> > > <natechancellor@...il.com> wrote:
> > > >
> > > > On Tue, Jan 12, 2021 at 04:19:01AM +0900, Masahiro Yamada wrote:
> > > > > On Tue, Jan 12, 2021 at 3:06 AM Nathan Chancellor
> > > > > <natechancellor@...il.com> wrote:
> > > > > >
> > > > > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for
> > > > > > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid
> > > > > > copy of pahole results in a kernel that will fully compile but fail to
> > > > > > link. The user then has to either install pahole or disable
> > > > > > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build
> > > > > > has failed, which could have been a significant amount of time depending
> > > > > > on the hardware.
> > > > > >
> > > > > > Avoid a poor user experience and require pahole to be installed with an
> > > > > > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is
> > > > > > standard for options that require a specific tools version.
> > > > > >
> > > > > > Suggested-by: Sedat Dilek <sedat.dilek@...il.com>
> > > > > > Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> > > > >
> > > > >
> > > > >
> > > > > I am not sure if this is the right direction.
> > > > >
> > > > >
> > > > > I used to believe moving any tool test to the Kconfig
> > > > > was the right thing to do.
> > > > >
> > > > > For example, I tried to move the libelf test to Kconfig,
> > > > > and make STACK_VALIDATION depend on it.
> > > > >
> > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com/
> > > > >
> > > > > It was rejected.
> > > > >
> > > > >
> > > > > In my understanding, it is good to test target toolchains
> > > > > in Kconfig (e.g. cc-option, ld-option, etc).
> > > > >
> > > > > As for host tools, in contrast, it is better to _intentionally_
> > > > > break the build in order to let users know that something needed is missing.
> > > > > Then, they will install necessary tools or libraries.
> > > > > It is just a one-time setup, in most cases,
> > > > > just running 'apt install' or 'dnf install'.
> > > > >
> > > > >
> > > > >
> > > > > Recently, a similar thing happened to GCC_PLUGINS
> > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/20201203125700.161354-1-masahiroy@kernel.org/#23855673
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Following this pattern, if a new pahole is not installed,
> > > > > it might be better to break the build instead of hiding
> > > > > the CONFIG option.
> > > > >
> > > > > In my case, it is just a matter of 'apt install pahole'.
> > > > > On some distributions, the bundled pahole is not new enough,
> > > > > and people may end up with building pahole from the source code.
> > > >
> > > > This is fair enough. However, I think that parts of this patch could
> > > > still be salvaged into something that fits this by making it so that if
> > > > pahole is not installed (CONFIG_PAHOLE_VERSION=0) or too old, the build
> > > > errors at the beginning, rather at the end. I am not sure where the best
> > > > place to put that check would be though.
> > >
> > > Me neither.
> > >
> > >
> > > Collecting tool checks to the beginning would be user-friendly.
> > > However, scattering the related code to multiple places is not
> > > nice from the developer point of view.
> > >
> > > How big is it a problem if the build fails
> > > at the very last stage?
> > >
> > > You can install pahole, then resume "make".
> > >
> > > Kbuild skips unneeded building, then you will
> > > be able to come back to the last build stage shortly.
> >
> > There will often be times where I am testing multiple configurations in
> > a row serially and the longer that a build takes to fail, the longer it
> > takes for me to get a "real" result. That is my motivation behind this
> > change. If people are happy with the current state of things, I will
> > just stick with universally disabling CONFIG_DEBUG_INFO_BTF in my test
> > framework.
> >
>
> I see where Masahiro is coming from. Not seeing CONFIG_DEBUG_INFO_BTF
> option because pahole is not installed (or is not new enough) is, I
> believe, for the majority of users, a much bigger confusion. Currently
> they will get a specific and helpful message at the link time, which
> is much more actionable, IMO. Once you fix pahole dependency, running
> make again would skip all the already compiled code and would start
> linking almost immediately, so if you are doing build locally there is
> a very little downside.

Hm.. Just saw Linus proposing using $(error-if) in Kconfig for an
unrelated issue ([0]). If we can make this work, then it would catch
such issue early on, yet won't have any downsides of hiding
CONFIG_DEBUG_INFO_BTF if pahole is too old. WDYT?

  [0] https://lore.kernel.org/lkml/CAHk-=wh-+TMHPTFo1qs-MYyK7tZh-OQovA=pP3=e06aCVp6_kA@mail.gmail.com/

>
> I understand your situation is a bit different in that you are
> building from scratch every single time (probably some sort of CI
> setup, right?). But it's a rarer and more power-user use case. And
> fixing pahole dependency is a one-time fix, so it's frustrating, but
> fixable on your side.
>
> As for disabling CONFIG_DEBUG_INFO_BTF. It's up to you and depends on
> what you are after, but major distros now enable it by default, so if
> you want to resemble common kernel configs, it's probably better to
> stick with it.
>
> Ideally, I'd love for Kconfig to have a way to express tool
> dependencies in such a way that it's still possible to choose desired
> options and if the build environment is lacking dependencies then it
> would be communicated early on. I have no idea if that's doable and
> how much effort it'd take, though.
>
>
> > Cheers,
> > Nathan

Powered by blists - more mailing lists