Re: [PATCH v4 13/22] modpost: cache section relocation mismatch state

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Oct 15, 2026 at 05:42:45PM +0200, Petr Pavlu wrote:
> On 9/23/26 7:18 PM, Lorenzo Stoakes (ARM) wrote:
> > For every relocation modpost invokes check_section_mismatch() to determine
> > whether there is any kind of mismatch between the source and destination,
> > and if so which classification applies.
> >
> > Each time it does this it invokes section_mismatch() which iterates
> > through the sectioncheck[] array every time it's called.
> >
> > When walked a relocation section the source is fixed and there aren't many
> > targets, so the same names are looked up over and over again.
> >
> > Therefore cache not only mismatch categorisation but also whether a
> > mismatch even exists for a given section and look up the sections in the
> > cache.
> >
> > Special indices (undefined, absolute, common) take the uncached path as
> > before.
> >
> > This results in very significant speed ups for allmodconfig incremental
> > builds.
> >
> > modpost is on the serial tail of every build that links vmlinux.
> >
> > Whole build, 128-thread Threadripper 9980X, best of N runs:
> >
> >                                          before   after     delta
> >                                          -------------------------------
> >   x86 defconfig, touch mm/vma.c, gcc        8.5s     8.3s    -0.14s (-2%)
> >   x86 defconfig, touch mm/vma.c, clang      7.7s     7.6s    -0.13s (-2%)
> >   x86 allmodconfig, touch mm/vma.c, gcc    33.9s    29.4s     -4.5s (-13%)
> >   x86 allmodconfig, touch mm/vma.c, clang  31.9s    27.9s     -4.0s (-13%)
> >
> > Assisted-by: LLM
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
> This looks mostly ok to me. Below are some nits you might want to
> consider addressing.

Thanks! :)

>
> > ---
> >  scripts/mod/modpost.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 64 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 75374c64b8cc..0fd43c8a89ea 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1155,12 +1155,67 @@ static void check_export_symbol(struct module *mod, struct elf_info *elf,
> >  			 name);
> >  }
> >
> > +/*
> > + * mismatch_cache[section index] ->
> > + *   0 - uncached.
> > + *  -1 - no mismatch.
> > + *  >0 - mismatch index + 1.
> > + */
> > +static int *mismatch_cache;
>
> It might be cleaner to maintain this cache per elf_info, since it
> logically belongs to the processing of an individual ELF file. This
> would also be consistent with the existing symsearch cache, which uses
> elf::symsearch.

Ack, makes sense, moved the decl inside elf_info.

>
> The description could use 'to_secndx' instead of 'section index' to
> clarify whether it refers to the source or destination section of
> a relocation.

Ack, updated for v5.

>
> > +
> > +static void init_mismatch_cache(unsigned int num_sections)
> > +{
> > +	mismatch_cache = xcalloc(num_sections, sizeof(*mismatch_cache));
> > +}
> > +
> > +static void reset_mismatch_cache(unsigned int num_sections)
> > +{
> > +	memset(mismatch_cache, 0, num_sections * sizeof(*mismatch_cache));
> > +}
> > +
> > +static void free_mismatch_cache(void)
> > +{
> > +	free(mismatch_cache);
> > +	mismatch_cache = NULL;
> > +}
> > +
> > +static const struct sectioncheck
> > +*cache_mismatch(unsigned int secndx, const struct sectioncheck *mismatch)
>
> Similarly here, s/secndx/to_secndx/.

Ack, done.

>
> > +{
> > +	if (!mismatch) {
> > +		mismatch_cache[secndx] = -1;
> > +		return NULL;
> > +	}
> > +
> > +	mismatch_cache[secndx] = (mismatch - sectioncheck) + 1;
> > +	return mismatch;
> > +}
> > +
> > +static const struct sectioncheck *get_section_mismatch(const char *fromsec,
> > +		const struct elf_info *elf, unsigned int secndx)
>
> s/secndx/to_secndx/, and the fromsec and elf arguments could be swapped
> for consistency with the rest of the code.

Ack, done.

>
> > +{
> > +	int cached;
> > +
> > +	if (secndx >= elf->num_sections)
> > +		return section_mismatch(fromsec, sec_name(elf, secndx));
> > +
> > +	cached = mismatch_cache[secndx];
> > +	if (cached < 0)
> > +		return NULL;
> > +	if (cached > 0)
> > +		return &sectioncheck[cached - 1];
> > +
> > +	return cache_mismatch(secndx,
> > +			      section_mismatch(fromsec, sec_name(elf, secndx)));
> > +}
> > +
> >  static void check_section_mismatch(struct module *mod, struct elf_info *elf,
> >  				   Elf_Sym *sym,
> >  				   unsigned int fsecndx, const char *fromsec,
> >  				   Elf_Addr faddr, Elf_Addr taddr)
> >  {
> > -	const char *tosec = sec_name(elf, get_secindex(elf, sym));
> > +	const unsigned int to_secndx = get_secindex(elf, sym);
>
> The const qualifier can be dropped from the to_secndx definition to
> match the usual kernel style.

Ack, done.

>
> --
> Thanks,
> Petr

Thanks for the review!

--
Cheers, Lorenzo