Re: [PATCH v4 14/22] modpost: emit module descriptors as assembly

[Petr Pavlu <petr.pavlu@suse.com>]

On 9/23/26 7:18 PM, Lorenzo Stoakes (ARM) wrote:
> modpost generates a descriptor for every module in the form of a
> <module>.mod.c file with .modinfo strings, the __this_module descriptor,
> exported symbol tables and (with CONFIG_MODVERSIONS set), the CRC of
> imported symbols.
> 
> These files are compiled like any other kernel C file with all of the
> -include preamble, as well as including linux/module.h, header dependencies
> generated by fixdep of a few hundred headers, an objtool run and if LTO is
> being performed, a link is performed to generate native code.
> 
> On an x86-64 allmodconfig build 11,189 *.mod.c files are built, each
> taking ~0.24s of CPU time to compile, and module finalisation as a whole
> 6,300 CPU seconds, or 64 seconds of wall time when run over 128 threads.
> 
> It also generates ~1.3 GiB of *.mod.o.cmd files that every subsequent build
> has to read back.
> 
> Avoid all this by emitting the descriptors as assembly instead.
> 
> The layout required (size and alignment of struct module, struct
> modversion_info, the module's name offsets, init, and exit fields and
> whether the architecture uses PREL32 ksymtab references) can all be derived
> from scripts/mod/module-offsets.h.
> 
> The fields of __this_module are emitted in offset order rather than
> declaration order, as CONFIG_RANDSTRUCT shuffles struct module so the name,
> init and exit fields can land anywhere.
> 
> An assembly file avoids all of the issues previously mentioned so this
> conversion results in a very significant performance win on kernel build.
> 
> As a consequence of this change, since module-offsets.c includes
> linux/module.h, scripts/mod is now built after the generated headers in
> prepare0, rather than before.
> 
> Also update .gitignore and make clean to handle .mod.S files, but keep
> .mod.c files there to ensure that users do not end up with untracked
> changes/dirty trees after the change takes effect.
> 
> The sections were confirmed to be byte-for-byte identical to the C version
> produced - each of .modinfo, .gnu.linkonce.this_module, __ksymtab*,
> __ksymtab_strings, __kcrctab*, __kflagstab*, __versions,
> __version_ext_crcs, __version_ext_names and their relocations - for all
> 8,135 modules of a clang allmodconfig build with CONFIG_COMPILE_TEST off
> and CONFIG_MODVERSIONS, CONFIG_EXTENDED_MODVERSIONS and
> CONFIG_MODULE_SRCVERSION_ALL on, and for a sample built with gcc.
> 
> What differs is what the compiler added around them: the __UNIQUE_ID_*
> locals, the KASAN constructor for the .mod.c globals, and on x86 a
> .note.gnu.property that the linker already drops from any module containing
> an assembly file and the loader never reads.
> 
> None of these have any impact on the build, however.
> 
> The assembly carries no debug information, so in order to keep describing
> __this_module in each module's debug information, define it weakly in
> module-common.c too, in a section which the module link discards.
> 
> x86_64 kernels built with gcc and clang, with CONFIG_MODVERSIONS,
> CONFIG_EXTENDED_MODVERSIONS and CONFIG_MODULE_SRCVERSION_ALL, were booted,
> every module and an external one loaded and unloaded, and the srcversions
> checked against modinfo, i386 and arm64 defconfigs (gcc and clang) and a
> ThinLTO build were also built as part of testing.
> 
> An x86_64 CONFIG_RANDSTRUCT_FULL build with clang, which places the name
> field after init and exit, was checked the same way and produces the same
> .gnu.linkonce.this_module sections and relocations as the C version.
> 
> On the x86_64 allmodconfig with clang 22, "make modules" with every
> *.mod.o and *.ko deleted goes from 64.5s (6,306 CPU-s) to 28.5s (518
> CPU-s).
> 
> A consequence of this change is that the make jobs are now so small that
> make cannot dispatch them quick enough, however the next commit in the
> series addresses this issue.
> 
> This impacts clean and no-op builds with a large number of modules most
> noticeably.
> 
> Whole build, 128-thread Threadripper 9980X, best of N runs:
> 
>                                          before   after     delta
>                                          -------------------------------
>   x86 allmodconfig, no-op make, gcc        12.7s     2.3s    -10.4s (-82%)
>   x86 allmodconfig, no-op make, clang      13.9s     2.8s    -11.2s (-80%)
>   x86 allmodconfig, clean, gcc            342.6s   306.6s    -36.0s (-11%)
>   x86 allmodconfig, clean, clang          340.1s   290.2s    -49.8s (-15%)
> 
> Assisted-by: LLM
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

This version looks mostly ok to me, from a modules perspective. Below
are a few nits plus one more useful comment about handling
MODULE_ARCH_INIT.

> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 0fd43c8a89ea..9d96459e492c 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> [...]
> +/* The name and namespace strings a ksymtab entry refers to. */
> +static void add_asm_kstrtab(struct buffer *buf, const struct symbol *sym)
> +{
> +	buf_printf(buf, "\t.section \"__ksymtab_strings\",\"aMS\",%%progbits,1\n");
> +	buf_printf(buf, "__kstrtab_%s:\n", sym->name);
> +	buf_asciz(buf, sym->name);
> +	buf_printf(buf, "__kstrtabns_%s:\n", sym->name);
> +	buf_asciz(buf, sym->namespace);
> +	buf_printf(buf, "\t.previous\n");
> +}
> +
> +/* The equivalent of SYMBOL_FLAGS(). */
> +static void add_asm_kflagstab(struct buffer *buf, const struct symbol *sym)
> +{
> +	buf_printf(buf, "\t.section \"___kflagstab+%s\", \"a\"\n", sym->name);
> +	buf_printf(buf, "__flags_%s:\n", sym->name);
> +	buf_printf(buf, "\t.byte 0x%02x\n", get_symbol_flags(sym));
> +	buf_printf(buf, "\t.previous\n");
> +}
> +
> +/* The equivalent of KSYMTAB_FUNC()/KSYMTAB_DATA(). */
> +static void add_asm_ksymtab(struct buffer *buf, const struct symbol *sym)
> +{
> +	const char *name = sym->name;
> +
> +	add_asm_kstrtab(buf, sym);
> +
> +	buf_printf(buf, "\t.section \"___ksymtab+%s\", \"a\"\n", name);
> +	buf_printf(buf, "\t.balign %d\n", KSYM_ALIGN);
> +	buf_printf(buf, "__ksymtab_%s:\n", name);
> +	add_asm_ksym_ref(buf, sym->is_func ? MOD_FUNC_PREFIX : "", name);
> +	add_asm_ksym_ref(buf, "__kstrtab_", name);
> +	add_asm_ksym_ref(buf, "__kstrtabns_", name);
> +	buf_printf(buf, "\t.previous\n");
> +
> +	add_asm_kflagstab(buf, sym);
> +}

add_asm_kstrtab() and add_asm_kflagstab() are only called from
add_asm_ksymtab() and their logic is fairly short. I suggest merging
these functions.

> +
> +/* The equivalent of SYMBOL_CRC(). */
> +static void add_asm_crc(struct buffer *buf, const struct symbol *sym)
> +{
> +	buf_printf(buf, "\t.section \"___kcrctab+%s\",\"a\"\n", sym->name);
> +	buf_printf(buf, "\t.balign 4\n");
> +	buf_printf(buf, "__crc_%s:\n", sym->name);
> +	buf_printf(buf, "\t.long 0x%08x\n", sym->crc);
> +	buf_printf(buf, "\t.previous\n");
> +}
> +
> +static bool export_is_kept(const struct symbol *sym)
> +{
> +	return !trim_unused_exports || sym->used;
> +}

export_is_kept() is only called once from add_asm_exported_symbols().
The condition can be inlined directly there.

> +
> +/* The exported symbols and, with modversions, their CRCs. */
> +static void add_asm_exported_symbols(struct buffer *buf, struct module *mod)
> +{
> +	struct symbol *sym;
> +
> +	list_for_each_entry(sym, &mod->exported_symbols, list) {
> +		if (!export_is_kept(sym))
> +			continue;
> +
> +		add_asm_ksymtab(buf, sym);
> +
> +		if (!modversions)
> +			continue;
> +
> +		if (!sym->crc_valid)
> +			mod_warn(mod, "EXPORT symbol '%s' version generation failed, symbol will not be versioned.\n"
> +				 "Is '%s' prototyped in <asm/asm-prototypes.h>?\n",
> +				 sym->name, sym->name);
> +		add_asm_crc(buf, sym);
> +	}
> +}
> +
> +/* Zero fill up to the offset. */
> +static void asm_skip_to(struct buffer *buf, unsigned int *pos,
> +			unsigned int offset)
> +{
> +	if (offset < *pos)
> +		fatal("__this_module field at offset %u overlaps the previous one\n",
> +		      offset);
> +	if (offset > *pos)
> +		buf_printf(buf, "\t.skip %u\n", offset - *pos);
> +
> +	*pos = offset;
> +}
> +
> +/* A non-zero field of __this_module: its offset and the name or symbol in it. */
> +struct this_module_field {
> +	unsigned int offset;
> +	bool is_name;
> +	const char *value;
> +};
> +
> +#define THIS_MODULE_MAX_FIELDS	5
> +
> +static int compare_field_offsets(const void *ptr_a, const void *ptr_b)
> +{
> +	const struct this_module_field *field_a = ptr_a, *field_b = ptr_b;
> +
> +	if (field_a->offset != field_b->offset)
> +		return field_a->offset < field_b->offset ? -1 : 1;
> +
> +	return 0;
> +}
> +
> +/*
> + * The fields of __this_module which are not zero, in offset order. The order
> + * is only known from module-offsets.h, CONFIG_RANDSTRUCT shuffles struct module.
> + */
> +static unsigned int get_this_module_fields(const struct module *mod,
> +					   const char *modname,
> +					   struct this_module_field *fields)
> +{
> +	unsigned int nr_fields = 0;
> +
> +	fields[nr_fields++] = (struct this_module_field)
> +		{ MOD_OFF_module_name, true, modname };
> +	if (mod->has_init)
> +		fields[nr_fields++] = (struct this_module_field)
> +			{ MOD_OFF_module_init, false, MOD_FUNC_PREFIX "init_module" };
> +#ifdef MOD_OFF_module_exit
> +	if (mod->has_cleanup)
> +		fields[nr_fields++] = (struct this_module_field)
> +			{ MOD_OFF_module_exit, false, MOD_FUNC_PREFIX "cleanup_module" };
> +#endif
> +#ifdef MOD_OFF_module_arch_fixup_start
> +	fields[nr_fields++] = (struct this_module_field)
> +		{ MOD_OFF_module_arch_fixup_start, false, "__start_fixup" };
> +	fields[nr_fields++] = (struct this_module_field)
> +		{ MOD_OFF_module_arch_fixup_end, false, "__stop_fixup" };
> +#endif
> +	qsort(fields, nr_fields, sizeof(*fields), compare_field_offsets);
> +
> +	return nr_fields;
> +}
> +
> +/* Emit one field of __this_module, returning its size. */
> +static unsigned int add_asm_this_module_field(struct buffer *buf,
> +					      const struct this_module_field *field)
> +{
> +	if (field->is_name) {
> +		buf_printf(buf, "\t.ascii \"%s\"\n", field->value);
> +		return strlen(field->value);
> +	}
> +
> +	buf_printf(buf, "\t" MOD_PTR_DIRECTIVE " %s\n", field->value);
> +	return MOD_SIZEOF_LONG;
> +}
> +
> +/*
> + * The equivalent of:
> + *
> + *	__visible struct module __this_module __section(".gnu.linkonce.this_module")
> + *		= { .name = KBUILD_MODNAME, .init = init_module, .exit = cleanup_module,
> + *		    .arch = MODULE_ARCH_INIT };
> + *
> + * Everything not listed is zero, MODULE_ARCH_INIT included, except on m68k.
> + */
> +static void add_asm_this_module(struct buffer *buf, const struct module *mod,
> +				const char *modname)
> +{
> +	struct this_module_field fields[THIS_MODULE_MAX_FIELDS];
> +	const unsigned int nr_fields = get_this_module_fields(mod, modname, fields);

The const qualifier can be removed from the nr_fields definition to
match the usual kernel style.

> +	unsigned int pos = 0, i;
> +
> +	buf_printf(buf, "\n\t.section .gnu.linkonce.this_module,\"aw\",%%progbits\n");
> +	buf_printf(buf, "\t.balign %d\n", MOD_ALIGNOF_struct_module);
> +	buf_printf(buf, "\t.globl __this_module\n");
> +	buf_printf(buf, "\t.type __this_module, %%object\n");
> +	buf_printf(buf, "\t.size __this_module, %d\n", MOD_SIZEOF_struct_module);
> +	buf_printf(buf, "__this_module:\n");
> +
> +	for (i = 0; i < nr_fields; i++) {
> +		asm_skip_to(buf, &pos, fields[i].offset);
> +		pos += add_asm_this_module_field(buf, &fields[i]);
> +	}
> +
> +	asm_skip_to(buf, &pos, MOD_SIZEOF_struct_module);
> +}
> +
> +/*
> + * An unresolved symbol without a module is not versioned; one without a CRC
> + * cannot be, so warn about it.
> + */
> +static bool skip_unversioned(struct module *mod, const struct symbol *sym)
> +{
> +	if (!sym->module)
> +		return true;
> +	if (sym->crc_valid)
> +		return false;
> +
> +	mod_warn(mod, "symbol '%s' has no CRC!\n", sym->name);
> +	return true;
> +}
> +
> +/* One struct modversion_info: the CRC, then the name padded to the end. */
> +static void add_asm_version(struct buffer *buf, const struct symbol *sym)
> +{
> +	buf_printf(buf, "\t" MOD_PTR_DIRECTIVE " 0x%08x\n", sym->crc);
> +	buf_printf(buf, "\t.ascii \"%s\"\n", sym->name);
> +	buf_printf(buf, "\t.skip %zu\n", MOD_SIZEOF_struct_modversion_info -
> +		   MOD_OFF_modversion_info_name - strlen(sym->name));
> +}
> +
> +/*
> + * The equivalent of:
> + *
> + *	static const struct modversion_info ____versions[]
> + *	__used __section("__versions") = { { crc, "name" }, ... };
> + *
> + * for unresolved symbols.
> + */
> +static void add_asm_versions(struct buffer *buf, struct module *mod)
> +{
> +	struct symbol *sym;
> +
> +	if (!basic_modversions)
> +		return;
> +
> +	buf_printf(buf, "\n\t.section __versions,\"a\",%%progbits\n");
> +	buf_printf(buf, "\t.balign %d\n", MOD_ALIGNOF_struct_modversion_info);
> +	list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> +		if (skip_unversioned(mod, sym))
> +			continue;
> +
> +		if (strlen(sym->name) >= MOD_NAME_LEN) {
> +			/* Only the extended table can hold it. */
> +			if (extended_modversions)
> +				continue;
> +
> +			mod_error(mod, "too long symbol '%s'\n", sym->name);
> +			break;
> +		}
> +
> +		add_asm_version(buf, sym);
> +	}
> +}
> +
> +/*
> + * The equivalent of:
> + *	static const u32 ____version_ext_crcs[] __section("__version_ext_crcs") = { crc, ... };
> + *	static const char ____version_ext_names[] __section("__version_ext_names") = "name\0" ...;
> + *
> + * for unresolved symbols.
> + */
> +static void add_asm_extended_versions(struct buffer *buf, struct module *mod)
> +{
> +	struct symbol *sym;
> +
> +	if (!extended_modversions)
> +		return;
> +
> +	buf_printf(buf, "\n\t.section __version_ext_crcs,\"a\",%%progbits\n");
> +	buf_printf(buf, "\t.balign 4\n");
> +	list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> +		if (skip_unversioned(mod, sym))
> +			continue;
> +
> +		buf_printf(buf, "\t.long 0x%08x\n", sym->crc);
> +	}
> +
> +	/*
> +	 * A symbol without a CRC was warned about above, and is skipped here
> +	 * too so that the names line up with the CRCs.
> +	 */
> +	buf_printf(buf, "\t.section __version_ext_names,\"a\",%%progbits\n");
> +	list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> +		if (!sym->module || !sym->crc_valid)
> +			continue;

This condition must match the logic in skip_unversioned(). It would be
clearer to keep it only in one place by calling skip_unversioned() here
and adding a "silent" parameter to the function.

> diff --git a/scripts/mod/module-offsets.c b/scripts/mod/module-offsets.c
> new file mode 100644
> index 000000000000..a336dd47aa33
> --- /dev/null
> +++ b/scripts/mod/module-offsets.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Layout of the structures modpost emits into *.mod.S, extracted from the
> + * target headers as devicetable-offsets.c does for the device tables.
> + */

The second part of the comment, after the comma, is superfluous. It is
expected that the various parts of modpost use consistent mechanisms
where appropriate.

> +#define COMPILE_OFFSETS
> +#include <linux/kbuild.h>
> +#include <linux/module.h>
> +
> +int main(void)
> +{
> +	DEFINE(MOD_SIZEOF_LONG, sizeof(long));
> +	DEFINE(MOD_PREL32_RELOCATIONS, IS_ENABLED(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS));
> +	DEFINE(MOD_FUNC_PLABEL, IS_ENABLED(CONFIG_PARISC) && IS_ENABLED(CONFIG_64BIT));
> +
> +	DEFINE(MOD_SIZEOF_struct_module, sizeof(struct module));
> +	DEFINE(MOD_ALIGNOF_struct_module, __alignof__(struct module));
> +	OFFSET(MOD_OFF_module_name, module, name);
> +	OFFSET(MOD_OFF_module_init, module, init);
> +#ifdef CONFIG_MODULE_UNLOAD
> +	OFFSET(MOD_OFF_module_exit, module, exit);
> +#endif
> +#if defined(CONFIG_M68K) && defined(CONFIG_MMU)
> +	/* MODULE_ARCH_INIT: the only architecture where it is not all zeroes. */
> +	OFFSET(MOD_OFF_module_arch_fixup_start, module, arch.fixup_start);
> +	OFFSET(MOD_OFF_module_arch_fixup_end, module, arch.fixup_end);
> +#endif

I feel it is a bit of a layering violation for modpost to know how
individual architectures need to initialize module::arch.

For now, this is probably ok. I would just remove MODULE_ARCH_INIT from
include/linux/module.h and arch/m68k/include/asm/module.h, along with
the comment referring to it in scripts/mod/modpost.c.

In the future, this could be avoided by having the m68k module support
look up the .m68k_fixup section at runtime, as other architectures
typically do for similar sections.

-- 
Thanks,
Petr