Message-ID: <b3phqbeo6twxkoxxmx7yxpas5egjdqcfxvaabub5wvlal373q7@jt4rhvwdiqsn>
Date: Thu, 1 Aug 2024 22:27:45 -0600
From: Jose Fernandez <jose.fernandez@...ux.dev>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Thomas Weißschuh <linux@...ssschuh.net>, 
	Christian Heusel <christian@...sel.eu>, Masahiro Yamada <masahiroy@...nel.org>, 
	Nicolas Schier <nicolas@...sle.eu>, Peter Jung <ptr1337@...hyos.org>, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kbuild: add debug package to pacman PKGBUILD

On 24/08/01 12:20PM, Nathan Chancellor wrote:
> On Thu, Aug 01, 2024 at 08:53:54PM +0200, Thomas Weißschuh wrote:
> > On 2024-08-01 11:36:37+0000, Nathan Chancellor wrote:
> > > Hi Jose,
> > > 
> > > On Thu, Aug 01, 2024 at 07:29:40AM -0600, Jose Fernandez wrote:
> > > > Add a new -debug package to the pacman PKGBUILD that will contain the
> > > > vmlinux image for debugging purposes. This package depends on the
> > > > -headers package and will be installed in /usr/src/debug/${pkgbase}.
> > > > 
> > > > The vmlinux image is needed to debug core dumps with tools like crash.
> > > > 
> > > > Signed-off-by: Jose Fernandez <jose.fernandez@...ux.dev>
> > > > Reviewed-by: Peter Jung <ptr1337@...hyos.org>
> > > 
> > > This appears to add a non-trivial amount of time to the build when benchmarking
> > > with Arch Linux's configuration (I measure 9% with hyperfine):
> > 
> > As nothing more is compiled, I guess this is just the additional
> > packaging.
> > 
> > > Benchmark 1: pacman-pkg @ 21b136cc63d2 ("minmax: fix up min3() and max3() too")
> > >   Time (mean ± σ):     579.541 s ±  0.585 s    [User: 22156.731 s, System: 3681.698 s]
> > >   Range (min … max):   578.894 s … 580.033 s    3 runs
> > > 
> > > Benchmark 2: pacman-pkg @ c5af4db0563b ("kbuild: add debug package to pacman PKGBUILD")
> > >   Time (mean ± σ):     633.419 s ±  0.972 s    [User: 22247.886 s, System: 3673.879 s]
> > >   Range (min … max):   632.302 s … 634.070 s    3 runs
> > > 
> > > Summary
> > >   pacman-pkg @ 21b136cc63d2 ("minmax: fix up min3() and max3() too") ran
> > >     1.09 ± 0.00 times faster than pacman-pkg @ c5af4db0563b ("kbuild: add debug package to pacman PKGBUILD")


Hi Nathan,
Thanks for taking the time to benchmark the change. I did notice that the build 
time for the -debug package felt a bit longer than I expected. I attributed it
to having to package a large file.

> > > It would be nice to add some option to avoid building this package for
> > > developers who may not want it (I know I personally would not want it
> > > with that penalty because I do a lot of bisects) or maybe adding a
> > > target to build this package with the rest like 'pacman-pkg-with-dbg' or
> > > something? 

My original idea was to add a make config to enable the -debug package, ie:
`make pacman-pkg DEBUG=1`. Your suggestion of a separate target is also a good
idea. I don't have strong opinion on this, so I'm open to what you and Thomas
prefer.

> > > Also, couldn't vmlinux be obtained from vmlinuz that already
> > > exists in the main package via scripts/extract-vmlinux?

I didn't know you could do that. However, I suspect that more people will 
encounter this issue and will want the vmlinux file available without knowing
you can extract it from vmlinuz. I think it's better to have it available 
explicitly in a package.

> > 
> > Jose:
> > 
> > In the vanilla PKGBUILD vmlinux is part of the linux-headers package:
> > linux-headers /usr/lib/modules/6.10.2-arch1-1/build/vmlinux
> > 
> > Given that you already gate the new -debug package on CONFIG_MODULES,
> > why not add the file to that package?

Hi Thomas,
My only concern with that approach is that it won't address Nathan's concern
about increasing the build time. I think an opt-in -debug package is the best
solution to address this.

Additionally, Peter from CachyOS (cced) shared this merge request with me:
https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/merge_requests/3

It looks like the vanilla PKGBUILD will start including a debug package as well.
I think it may be good to align with the pattern they will be using.

> > 
> > Then we could still discuss if it makes sense to gate vmlinux on
> > CONFIG_MODULES or if -headers should be enabled unconditionally again.
> 
> I think having -headers be enabled/built by default is probably okay
> since a regular user/builder might have external modules that need to be
> rebuilt against the new kernel. However, I (and likely many other
> upstream developers, however many use Arch) never build external modules
> against my kernels, so it would be nice to have some way to opt out of
> this penalty. I suspect it is just compressing down such a massive
> vmlinux that causes this? I have not had access to a build log with my
> testing, so unsure.
> 
> > Or we wait for somebody to show up and ask for it.
> 
> I won't insist though so if we want to wait for someone else, that's
> fine with me too.

I will lean on your preference for how to proceed with this. I'm happy to make
the changes you think are best. To me, it appears that the next iteration to 
satisfy everyone is to make the -debug package opt-in via a new make target or
a make config option.

Thanks,
Jose

> 
> > > Cheers,
> > > Nathan
> > > 
> > > > ---
> > > >  scripts/package/PKGBUILD | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/scripts/package/PKGBUILD b/scripts/package/PKGBUILD
> > > > index 663ce300dd06..beda3db21863 100644
> > > > --- a/scripts/package/PKGBUILD
> > > > +++ b/scripts/package/PKGBUILD
> > > > @@ -6,6 +6,7 @@ pkgbase=${PACMAN_PKGBASE:-linux-upstream}
> > > >  pkgname=("${pkgbase}" "${pkgbase}-api-headers")
> > > >  if grep -q CONFIG_MODULES=y include/config/auto.conf; then
> > > >  	pkgname+=("${pkgbase}-headers")
> > > > +	pkgname+=("${pkgbase}-debug")
> > > >  fi
> > > >  pkgver="${KERNELRELEASE//-/_}"
> > > >  # The PKGBUILD is evaluated multiple times.
> > > > @@ -89,6 +90,15 @@ _package-headers() {
> > > >  	ln -sr "${builddir}" "${pkgdir}/usr/src/${pkgbase}"
> > > >  }
> > > >  
> > > > +_package-debug(){
> > > > +    pkgdesc="Non-stripped vmlinux file for the ${pkgdesc} kernel"
> > > > +    depends=(${pkgbase}-headers)
> > > > +
> > > > +    cd "${objtree}"
> > > > +    mkdir -p "$pkgdir/usr/src/debug/${pkgbase}"
> > > > +    install -Dt "$pkgdir/usr/src/debug/${pkgbase}" -m644 vmlinux
> > > > +}
> > > > +
> > > >  _package-api-headers() {
> > > >  	pkgdesc="Kernel headers sanitized for use in userspace"
> > > >  	provides=(linux-api-headers)
> > > > -- 
> > > > 2.46.0
> > > > 

Powered by blists - more mailing lists