Re: [PATCH v2 13/21] modpost: emit module descriptors as assembly

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

On 9/14/26 11:22 AM, 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.

As a side note, I'm currently working on cleaning up what linux/module.h
includes. This was also originally motivated by an effort to reduce the
compilation time of .mod.c files, though cleaning up that header makes
sense in general, since it is the most explicitly included file in the
kernel.

> 
> 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.

Lowering the data to assembly loses debug information for __this_module,
specifically that it has type `struct module`. It might be worth
restoring that information in same way. A small hack would be to add
something like the following to scripts/module-common.c:

__weak __section(".discard.this_module") struct module __this_module;

> diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
> index 01a37ec872b9..75e9effdf02c 100644
> --- a/scripts/Makefile.modfinal
> +++ b/scripts/Makefile.modfinal
> @@ -20,10 +20,14 @@ __modfinal: $(modules:%.o=%.ko)
>  modname = $(notdir $(@:.mod.o=))
>  part-of-module = y
>  GCOV_PROFILE := n
> -ccflags-remove-y := $(CC_FLAGS_CFI)
>  
> -%.mod.o: %.mod.c FORCE
> -	$(call if_changed_rule,cc_o_c)
> +# modpost lays the <module>.mod.S out completely (write_mod_S_file()), so it
> +# needs only the assembler and no dependency tracking.
> +quiet_cmd_as_mod_o = AS [M]  $@
> +      cmd_as_mod_o = $(CC) $(_a_flags) $(modkern_aflags) -c -o $@ $<
> +
> +%.mod.o: %.mod.S FORCE
> +	$(call if_changed,as_mod_o)
>  
>  .module-common.o: $(srctree)/scripts/module-common.c FORCE
>  	$(call if_changed_rule,cc_o_c)

If the <module>.mod.S files are fully self-contained and do not even use
something like KBUILD_MODNAME, would it be possible to name them
<module>.mod.s and skip the C preprocessor as well?

> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 0fd43c8a89ea..b4550b545330 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2191,30 +2042,483 @@ static void write_vmlinux_export_c_file(struct module *mod)
>  	free(buf.p);
>  }
>  
> -/* do sanity checks, and generate *.mod.c file */
> -static void write_mod_c_file(struct module *mod)
> +#if MOD_SIZEOF_LONG == 8
> +#define MOD_PTR_DIRECTIVE	".quad"
> +#else
> +#define MOD_PTR_DIRECTIVE	".long"
> +#endif
> +
> +/* See KSYM_FUNC() in include/linux/export-internal.h. */
> +#if MOD_FUNC_PLABEL
> +#define MOD_FUNC_PREFIX		"P%"
> +#else
> +#define MOD_FUNC_PREFIX		""
> +#endif
> +
> +/* See __KSYM_ALIGN in include/linux/export-internal.h. */
> +#if MOD_PREL32_RELOCATIONS || MOD_SIZEOF_LONG == 4
> +#define KSYM_ALIGN		4
> +#else
> +#define KSYM_ALIGN		8
> +#endif
> +
> +/* Append the body of an assembler string literal, escaped as needed. */
> +static void buf_escaped(struct buffer *buf, const char *str)
> +{
> +	unsigned char chr;
> +
> +	while ((chr = *str++)) {
> +		if (chr == '"' || chr == '\\')
> +			buf_printf(buf, "\\%c", chr);
> +		else if (isprint(chr))
> +			buf_printf(buf, "%c", chr);
> +		else
> +			buf_printf(buf, "\\%03o", chr);
> +	}
> +}
> +
> +static void buf_asciz(struct buffer *buf, const char *str)
>  {
> -	struct buffer buf = { };
> -	struct module_alias *alias, *next;
> -	char fname[PATH_MAX];
> -	int ret;
> +	buf_printf(buf, "\t.asciz \"");
> +	buf_escaped(buf, str);
> +	buf_printf(buf, "\"\n");
> +}
>  
> -	add_header(&buf, mod);
> -	add_exported_symbols(&buf, mod);
> -	add_versions(&buf, mod);
> -	add_extended_versions(&buf, mod);
> -	add_depends(&buf, mod);
> +/* The equivalent of MODULE_INFO(tag, info). */
> +static void add_asm_modinfo(struct buffer *buf, const char *tag,
> +			    const char *info)
> +{
> +	buf_printf(buf, "\t.section .modinfo,\"a\",%%progbits\n");
> +	buf_printf(buf, "\t.asciz \"%s=", tag);
> +	buf_escaped(buf, info);
> +	buf_printf(buf, "\"\n");
> +}
>  
> -	buf_printf(&buf, "\n");
> +/* See __KSYM_REF() in include/linux/export-internal.h. */
> +static void add_asm_ksym_ref(struct buffer *buf, const char *prefix,
> +			     const char *sym)
> +{
> +#if MOD_PREL32_RELOCATIONS
> +	buf_printf(buf, "\t.long %s%s - .\n", prefix, sym);
> +#else
> +	buf_printf(buf, "\t" MOD_PTR_DIRECTIVE " %s%s\n", prefix, sym);
> +#endif
> +}
> +
> +/* 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);
> +}
> +
> +/* 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");
> +}

It would be good to avoid duplicating these symbol-related
implementations in include/linux/export-internal.h and
scripts/mod/modpost.c. Perhaps .vmlinux.export.c could also be lowered
to assembly, so the implementation in include/linux/export-internal.h
can be removed and scripts/mod/modpost.c becomes the only source of
truth.

> +
> +static bool export_is_kept(const struct symbol *sym)
> +{
> +	return !trim_unused_exports || sym->used;
> +}
> +
> +/* Record the CRCs of the exported symbols. */
> +static void add_asm_crcs(struct buffer *buf, struct module *mod)
> +{
> +	struct symbol *sym;
> +
> +	list_for_each_entry(sym, &mod->exported_symbols, list) {
> +		if (!export_is_kept(sym))
> +			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);
> +	}
> +}
> +
> +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))
> +			add_asm_ksymtab(buf, sym);
> +	}
> +
> +	if (modversions)
> +		add_asm_crcs(buf, mod);
> +}

Nit: This could be kept shorter as a single function, similarly to how
the original C version was implemented solely as add_exported_symbols().

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 (trim_unused_exports && !sym->used)
			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);
	}
}

> +/*
> + * 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);
> +	}
> +}
> +
> +static void add_asm_version_ext_crcs(struct buffer *buf, struct module *mod)
> +{
> +	struct symbol *sym;
> +
> +	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 with the CRCs, and is skipped here
> + * too so that the names line up with them.
> + */
> +static void add_asm_version_ext_names(struct buffer *buf, struct module *mod)
> +{
> +	struct symbol *sym;
> +
> +	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;
> +
> +		buf_asciz(buf, sym->name);
> +	}
> +	/* The terminator of the string literal this used to be. */
> +	buf_printf(buf, "\t.byte 0\n");

This is ok since the conversion aims to be byte-identical. In the
future, I believe the extra NUL byte should be removed and
elf_validity_cache_index_versions() also be updated to report when
__version_ext_names contains more names than there are CRCs in
__version_ext_crcs.

> +}
> +
> +/*
> + * 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)
> +{
> +	if (!extended_modversions)
> +		return;
> +
> +	add_asm_version_ext_crcs(buf, mod);
> +	add_asm_version_ext_names(buf, mod);
> +}

Nit: add_asm_version_ext_crcs() and add_asm_version_ext_names() are both
fairly short and are called only from add_asm_extended_versions(), so
I think they could be inlined into that function. This would be similar
to the original implementation, which does all the work in
add_extended_versions().

> +
> +/* Clear ->seen of the modules that own symbols this one needs. */
> +static void clear_seen_dependencies(struct module *mod)
> +{
> +	struct symbol *sym;
> +
> +	list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> +		if (sym->module)
> +			sym->module->seen = sym->module->is_vmlinux;
> +	}
> +}
> +
> +/* The modules this one depends on, each once, comma separated. */
> +static void collect_dependencies(struct module *mod, struct buffer *deps)
> +{
> +	struct symbol *sym;
> +	bool first = true;
> +
> +	clear_seen_dependencies(mod);
> +
> +	list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> +		struct module *owner = sym->module;
> +
> +		if (!owner || owner->seen)
> +			continue;
> +
> +		owner->seen = true;
> +		buf_printf(deps, "%s%s", first ? "" : ",",
> +			   get_basename(owner->name));
> +		first = false;
> +	}
> +	buf_write(deps, "", 1);
> +}
> +
> +static void add_asm_depends(struct buffer *buf, struct module *mod)
> +{
> +	struct buffer deps = { };
> +
> +	collect_dependencies(mod, &deps);
> +	buf_printf(buf, "\n");
> +	add_asm_modinfo(buf, "depends", deps.p);
> +	free(deps.p);
> +}

Nit: Same here. I think this would be more straightforward if
collect_dependencies() and clear_seen_dependencies() were inlined into
add_asm_depends(), matching the original add_depends().

-- 
Thanks,
Petr