Re: [PATCH v2] arm64: Allow CONFIG_AUTOFDO_CLANG to be selected

[Will Deacon <will@kernel.org>]

On Mon, Dec 09, 2024 at 03:51:34PM -0800, Yabin Cui wrote:
>  On Mon, Dec 9, 2024 at 10:56 AM Will Deacon <will@kernel.org> wrote:
> >
> > (Aside: please try to avoid top-posting on the public lists as it messes up
> > the flow of conversation; I'll try to piece this back together.)
> >
> > On Mon, Dec 09, 2024 at 09:30:50AM -0800, Rong Xu wrote:
> > > On Mon, Dec 9, 2024 at 8:20 AM Will Deacon <will@kernel.org> wrote:
> > > > On Mon, Nov 18, 2024 at 02:25:40PM -0800, Yabin Cui wrote:
> > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > > index fd9df6dcc593..c3814df5e391 100644
> > > > > --- a/arch/arm64/Kconfig
> > > > > +++ b/arch/arm64/Kconfig
> > > > > @@ -103,6 +103,7 @@ config ARM64
> > > > >       select ARCH_SUPPORTS_PER_VMA_LOCK
> > > > >       select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> > > > >       select ARCH_SUPPORTS_RT
> > > > > +     select ARCH_SUPPORTS_AUTOFDO_CLANG
> > > > >       select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
> > > > >       select ARCH_WANT_COMPAT_IPC_PARSE_VERSION if COMPAT
> > > > >       select ARCH_WANT_DEFAULT_BPF_JIT
> > > >
> > > > After this change, both arm64 and x86 select this option unconditionally
> > > > and with no apparent support code being added. So what is actually
> > > > required in order to select ARCH_SUPPORTS_AUTOFDO_CLANG and why isn't
> > > > it just available for all architectures instead?
> 
> I think it's similar to ARCH_SUPPORTS_LTO_CLANG, which also doesn't need any
> support code but requires testing to ensure it works on a specific architecture.
> 
> >
> > > Enabling an AutoFDO build requires users to explicitly set CONFIG_AUTOFDO_CLANG.
> > > The support code is in Commit 315ad8780a129e82 (kbuild: Add AutoFDO
> > > support for Clang build).
> >
> > Yes, that is precisely my point. The user has to enable
> > CONFIG_AUTOFDO_CLANG anyway, so what is the point in having
> > ARCH_SUPPORTS_AUTOFDO_CLANG. Why would an architecture _not_ want to
> > select that?
> >
> > > We are not enabling this for all architectures because AutoFDO's optimized build
> > > relies on Last Branch Records (LBR) which aren't available on all architectures.
> >
> > So? ETM isn't available on all arm64 machines and I doubt whether LBR is
> > available on _all_ x86 machines either. So there's a runtime failure
> > mode that needs to be handled anyway and I don't think the arch-specific
> > Kconfig option is really doing anything useful.
> 
> My understanding of the benefits of ARCH_SUPPORTS_AUTOFDO_CLANG is:
> 1. Generally, we don't prefer to collect an AutoFDO profile on one
> architecture and use it to build the kernel for another architecture.
> This is because the profile misses data for architecture-dependent
> code. ARCH_SUPPORTS_AUTOFDO_CLANG can partially prevent this from
> happening.

Hmm, not really. Once more than one architecture selects the option, you
have the possibility of the mismatch you're trying to avoid.

> 2. Building a kernel with an AutoFDO profile involves using new
> optimization flags for clang.  Having ARCH_SUPPORTS_AUTOFDO_CLANG=y
> for one architecture means someone has tested building a kernel with
> an AutoFDO profile on this architecture.

On the flip side, allowing all architectures to select the option
actually increases your test coverage.

Will