Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231018151950.205265-1-masahiroy@kernel.org> <20231018151950.205265-4-masahiroy@kernel.org>
 <ZTDlrkTXnkVN1cff@krava> <CAEf4BzZm4h4q6k9ZhuT5qiWC9PYA+c7XwVFd68iAq4mtMJ-qhw@mail.gmail.com>
 <CAK7LNAR2kKwbzdFxfVXDxsy8pfyQDCR-BN=zpbcZg0JS9RpsKQ@mail.gmail.com>
 <CAEf4BzbYwEFSNTFjJyhYmOOK5iwHjFAdcArkUbcQz5ntRvOOvA@mail.gmail.com> <CAK7LNAQxFgOpuCBYPSx5Z6aw5MtKzPL39XLUvZuUBSyRGnOZUg@mail.gmail.com>
In-Reply-To: <CAK7LNAQxFgOpuCBYPSx5Z6aw5MtKzPL39XLUvZuUBSyRGnOZUg@mail.gmail.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Sat, 21 Oct 2023 12:33:05 -0700
Message-ID: <CAEf4BzZqpqo3j33FkH3QJwezbJwarr1dXs4fCsp5So12_5MmTg@mail.gmail.com>
Subject: Re: [bpf-next PATCH v2 4/4] kbuild: refactor module BTF rule
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     Jiri Olsa <olsajiri@gmail.com>, linux-kbuild@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>, bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Oct 21, 2023 at 4:38=E2=80=AFAM Masahiro Yamada <masahiroy@kernel.o=
rg> wrote:
>
> On Sat, Oct 21, 2023 at 5:52=E2=80=AFAM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Oct 20, 2023 at 12:03=E2=80=AFAM Masahiro Yamada <masahiroy@ker=
nel.org> wrote:
> > >
> > > On Fri, Oct 20, 2023 at 7:55=E2=80=AFAM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Thu, Oct 19, 2023 at 1:15=E2=80=AFAM Jiri Olsa <olsajiri@gmail.c=
om> wrote:
> > > > >
> > > > > On Thu, Oct 19, 2023 at 12:19:50AM +0900, Masahiro Yamada wrote:
> > > > > > newer_prereqs_except and if_changed_except are ugly hacks of th=
e
> > > > > > newer-prereqs and if_changed in scripts/Kbuild.include.
> > > > > >
> > > > > > Remove.
> > > > > >
> > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > >   - Fix if_changed_except to if_changed
> > > > > >
> > > > > >  scripts/Makefile.modfinal | 25 ++++++-------------------
> > > > > >  1 file changed, 6 insertions(+), 19 deletions(-)
> > > > > >
> > > > > > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfi=
nal
> > > > > > index 9fd7a26e4fe9..fc07854bb7b9 100644
> > > > > > --- a/scripts/Makefile.modfinal
> > > > > > +++ b/scripts/Makefile.modfinal
> > > > > > @@ -19,6 +19,9 @@ vmlinux :=3D
> > > > > >  ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> > > > > >  ifneq ($(wildcard vmlinux),)
> > > > > >  vmlinux :=3D vmlinux
> > > > > > +cmd_btf =3D ; \
> > > > > > +     LLVM_OBJCOPY=3D"$(OBJCOPY)" $(PAHOLE) -J $(PAHOLE_FLAGS) =
--btf_base vmlinux $@; \
> > > > > > +     $(RESOLVE_BTFIDS) -b vmlinux $@
> > > > > >  else
> > > > > >  $(warning Skipping BTF generation due to unavailability of vml=
inux)
> > > > > >  endif
> > > > > > @@ -41,27 +44,11 @@ quiet_cmd_ld_ko_o =3D LD [M]  $@
> > > > > >        cmd_ld_ko_o +=3D                                        =
         \
> > > > > >       $(LD) -r $(KBUILD_LDFLAGS)                               =
       \
> > > > > >               $(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)       =
       \
> > > > > > -             -T scripts/module.lds -o $@ $(filter %.o, $^)
> > > > > > +             -T scripts/module.lds -o $@ $(filter %.o, $^)    =
       \
> > > > > > +     $(cmd_btf)
> > > > > >
> > > > > > -quiet_cmd_btf_ko =3D BTF [M] $@
> > > > >
> > > > > nit not sure it's intentional but we no longer display 'BTF [M] .=
..ko' lines,
> > > > > I don't mind not displaying that, but we should mention that in c=
hangelog
> > > > >
> > > >
> > > > Thanks for spotting this! I think those messages are useful and
> > > > important to keep. Masahiro, is it possible to preserve them?
> > >
> > >
> > >
> > > No, I do not think so.
> > >
> >
> > That's too bad, I think it's a useful one.
>
>
>
> I prioritize that the code is correct.
>

Could you please also prioritize not regressing informativeness of a
build log? With your changes it's not clear now if BTF was generated
or not for a kernel module, while previously it was obvious and was
easy to spot if for some reason BTF was not generated. I'd like to
preserve this
property, thank you.

E.g, can we still have BTF generation as a separate command and do a
separate $(call if_changed,btf_ko)? Or something along those lines.
Would that work?

>
>
> >
> > > Your code is wrong.
> > >
> >
> > Could be, but note the comment you are removing:
> >
> > # Re-generate module BTFs if either module's .ko or vmlinux changed
> >
> > BTF has to be re-generated not just when module .ko is regenerated,
> > but also when the vmlinux image itself changes.
> >
> > I don't see where this is done with your changes. Can you please point
> > it out explicitly?
>
>
>
> That is too obvious; %.ko depends on $(vmlinux).

Thank you for your gracious answer. We used to not rebuild module's
.ko's when vmlinux didn't change (but we did regen BTFs), and that's
why I was confused. Now we forcefully recompile modules, which is a
change in behavior which would be nice to call out in the commit
message.


>
>
>
> %.ko: %.o %.mod.o scripts/module.lds $(vmlinux) FORCE
>
>
>
>
> --
> Best Regards
> Masahiro Yamada