[PATCH 1/3] libkmod: Prefer builtin index over builtin.alias

[PATCH 1/3] libkmod: Prefer builtin index over builtin.alias

[Lucas De Marchi]

The modules.builtin.alias.bin is way larger than the
modules.builtin.bin.  On a normal "distro kernel":

	21k modules.builtin.alias.bin
	11k modules.builtin.bin

From the kernel we get both modules.builtin and modules.builtin.modinfo.
depmod generates modules.builtin.bin and modules.builtin.alias.bin
from them respectively. modules.bultin is not going away: it's not
deprecated by the new index added. So, let's just stop duplicating the
information inside modules.builtin.alias.bin and just use the other
index.
---
 libkmod/libkmod-module.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 6f7747c..40d394a 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -576,13 +576,14 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
 	err = kmod_lookup_alias_from_aliases_file(ctx, alias, list);
 	CHECK_ERR_AND_FINISH(err, fail, list, finish);
 
+	DBG(ctx, "lookup modules.builtin %s\n", alias);
+	err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
+
 	DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
 	err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
-	if (err == -ENOSYS) {
-		/* Optional index missing, try the old one */
-		DBG(ctx, "lookup modules.builtin %s\n", alias);
-		err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
-	}
+	/* Optional index missing, ignore */
+	if (err == -ENOSYS)
+		err = 0;
 	CHECK_ERR_AND_FINISH(err, fail, list, finish);
 
 
-- 
2.35.1

[PATCH 2/3] depmod: Do not duplicate builtin index

[Lucas De Marchi]

Now that libkmod uses modules.builtin.bin again, we don't need to add
the module names in modules.builtin.alias.bin and just add the aliases.

After this change, here are the new sizes for the indexes:

Before	After	index
21k	6.4K	modules.builtin.alias.bin
11k	 11K	modules.builtin.bin
---
 tools/depmod.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index eb810b8..ac6ead8 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -2405,7 +2405,7 @@ static int output_devname(struct depmod *depmod, FILE *out)
 
 static int output_builtin_alias_bin(struct depmod *depmod, FILE *out)
 {
-	int ret = 0, count = 0;
+	int ret = 0;
 	struct index_node *idx;
 	struct kmod_list *l, *builtin = NULL;
 
@@ -2450,9 +2450,6 @@ static int output_builtin_alias_bin(struct depmod *depmod, FILE *out)
 		}
 
 		kmod_module_info_free_list(info_list);
-
-		index_insert(idx, modname, modname, 0);
-		count++;
 	}
 
 out:
-- 
2.35.1

[PATCH 3/3] depmod: Stop opening modules.modinfo once per module

[Lucas De Marchi]

Since the addition of modules.aliases.bin, depmod has to open that
index multiple times and parse it over and over again:

	$ sudo strace -e openat  ./tools/depmod 2>&1 | grep modules.builtin.modinfo  | wc -l
	299
	$ time sudo  ./tools/depmod
	real    0m7.814s
	user    0m7.571s
	sys     0m0.237s

Rework the logic in depmod so it does everything: open, read and parse. The
format is very straightforward and we don't need to keep it in a data structure
since we only want to add the result to a index. New output:

	$ sudo strace -e openat  ./tools/depmod 2>&1 | grep modules.builtin.modinfo  | wc -l
	1
	$ time sudo  ./tools/depmod
	real    0m7.663s
	user    0m7.516s
	sys     0m0.139s

Indexes still match:

	$ cmp /tmp/modules.builtin.alias.bin.new
	/tmp/modules.builtin.alias.bin.old; echo $?
	0

Fix: https://github.com/kmod-project/kmod/issues/11
---
 tools/depmod.c | 158 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 96 insertions(+), 62 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index ac6ead8..07a35ba 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -2346,6 +2346,102 @@ static int output_builtin_bin(struct depmod *depmod, FILE *out)
 	return 0;
 }
 
+static int flush_stream(FILE *in, int endchar)
+{
+	size_t i = 0;
+	int c;
+
+	for (c = fgetc(in);
+	     c != EOF && c != endchar && c != '\0';
+	     c = fgetc(in))
+		;
+
+	return c == endchar ? i : 0;
+}
+
+static int flush_stream_to(FILE *in, int endchar, char *dst, size_t dst_sz)
+{
+	size_t i = 0;
+	int c;
+
+	for (c = fgetc(in);
+	     c != EOF && c != endchar && c != '\0' && i < dst_sz;
+	     c = fgetc(in))
+		dst[i++] = c;
+
+	if (i == dst_sz) {
+		WRN("Could not flush stream: %d. Partial content: %.*s\n",
+		    ENOSPC, (int) dst_sz, dst);
+	}
+
+	return c == endchar ? i : 0;
+}
+
+static int output_builtin_alias_bin(struct depmod *depmod, FILE *out)
+{
+	FILE *in;
+	struct index_node *idx;
+	int ret;
+
+	if (out == stdout)
+		return 0;
+
+	in = dfdopen(depmod->cfg->dirname, "modules.builtin.modinfo", O_RDONLY, "r");
+	if (in == NULL)
+		return 0;
+
+	idx = index_create();
+	if (idx == NULL) {
+		fclose(in);
+		return -ENOMEM;
+	}
+
+	/* format: modname.key=value\0 */
+	while (!feof(in) && !ferror(in)) {
+		char alias[PATH_MAX];
+		char modname[PATH_MAX];
+		char value[PATH_MAX];
+		size_t len;
+
+		len = flush_stream_to(in, '.', modname, sizeof(modname));
+		modname[len] = '\0';
+		if (!len)
+			continue;
+
+		len = flush_stream_to(in, '=', value, sizeof(value));
+		value[len] = '\0';
+		if (!streq(value, "alias")) {
+			flush_stream(in, '\0');
+			continue;
+		}
+
+		len = flush_stream_to(in, '\0', value, sizeof(value));
+		value[len] = '\0';
+		if (!len)
+			continue;
+
+		alias[0] = '\0';
+		if (alias_normalize(value, alias, NULL) < 0) {
+			WRN("Unmatched bracket in %s\n", value);
+			continue;
+		}
+
+		index_insert(idx, alias, modname, 0);
+	}
+
+	if (ferror(in)) {
+		ret = -EINVAL;
+	} else {
+		index_write(idx, out);
+		ret = 0;
+	}
+
+	index_destroy(idx);
+	fclose(in);
+
+	return ret;
+}
+
 static int output_devname(struct depmod *depmod, FILE *out)
 {
 	size_t i;
@@ -2403,68 +2499,6 @@ static int output_devname(struct depmod *depmod, FILE *out)
 	return 0;
 }
 
-static int output_builtin_alias_bin(struct depmod *depmod, FILE *out)
-{
-	int ret = 0;
-	struct index_node *idx;
-	struct kmod_list *l, *builtin = NULL;
-
-	if (out == stdout)
-		return 0;
-
-	idx = index_create();
-	if (idx == NULL)
-		return -ENOMEM;
-
-	ret = kmod_module_get_builtin(depmod->ctx, &builtin);
-	if (ret < 0) {
-		if (ret == -ENOENT)
-			ret = 0;
-		goto out;
-	}
-
-	kmod_list_foreach(l, builtin) {
-		struct kmod_list *ll, *info_list = NULL;
-		struct kmod_module *mod = l->data;
-		const char *modname = kmod_module_get_name(mod);
-
-		ret = kmod_module_get_info(mod, &info_list);
-		if (ret < 0)
-			goto out;
-
-		kmod_list_foreach(ll, info_list) {
-			char alias[PATH_MAX];
-			const char *key = kmod_module_info_get_key(ll);
-			const char *value = kmod_module_info_get_value(ll);
-
-			if (!streq(key, "alias"))
-				continue;
-
-			alias[0] = '\0';
-			if (alias_normalize(value, alias, NULL) < 0) {
-				WRN("Unmatched bracket in %s\n", value);
-				continue;
-			}
-
-			index_insert(idx, alias, modname, 0);
-		}
-
-		kmod_module_info_free_list(info_list);
-	}
-
-out:
-	/* do not bother writing the index if we are going to discard it */
-	if (ret > 0)
-		index_write(idx, out);
-
-	if (builtin)
-		kmod_module_unref_list(builtin);
-
-	index_destroy(idx);
-
-	return ret;
-}
-
 static int depmod_output(struct depmod *depmod, FILE *out)
 {
 	static const struct depfile {
-- 
2.35.1

Re: [PATCH 1/3] libkmod: Prefer builtin index over builtin.alias

[Alexey Gladkov]

On Fri, Feb 11, 2022 at 12:42:28AM -0800, Lucas De Marchi wrote:
> The modules.builtin.alias.bin is way larger than the
> modules.builtin.bin.  On a normal "distro kernel":
> 
> 	21k modules.builtin.alias.bin
> 	11k modules.builtin.bin
> 
> >From the kernel we get both modules.builtin and modules.builtin.modinfo.
> depmod generates modules.builtin.bin and modules.builtin.alias.bin
> from them respectively. modules.bultin is not going away: it's not
> deprecated by the new index added. So, let's just stop duplicating the
> information inside modules.builtin.alias.bin and just use the other
> index.
> ---
>  libkmod/libkmod-module.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 6f7747c..40d394a 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -576,13 +576,14 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
>  	err = kmod_lookup_alias_from_aliases_file(ctx, alias, list);
>  	CHECK_ERR_AND_FINISH(err, fail, list, finish);
>  
> +	DBG(ctx, "lookup modules.builtin %s\n", alias);
> +	err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> +

assigning to the err variable looks useless. It will be overwritten
anyway.

>  	DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
>  	err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> -	if (err == -ENOSYS) {
> -		/* Optional index missing, try the old one */
> -		DBG(ctx, "lookup modules.builtin %s\n", alias);
> -		err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> -	}
> +	/* Optional index missing, ignore */
> +	if (err == -ENOSYS)
> +		err = 0;
>  	CHECK_ERR_AND_FINISH(err, fail, list, finish);
>  
>  
> -- 
> 2.35.1
> 

-- 
Rgrds, legion

Re: [PATCH 1/3] libkmod: Prefer builtin index over builtin.alias

[Lucas De Marchi]

On Fri, Feb 11, 2022 at 4:28 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> On Fri, Feb 11, 2022 at 12:42:28AM -0800, Lucas De Marchi wrote:
> > The modules.builtin.alias.bin is way larger than the
> > modules.builtin.bin.  On a normal "distro kernel":
> >
> >       21k modules.builtin.alias.bin
> >       11k modules.builtin.bin
> >
> > >From the kernel we get both modules.builtin and modules.builtin.modinfo.
> > depmod generates modules.builtin.bin and modules.builtin.alias.bin
> > from them respectively. modules.bultin is not going away: it's not
> > deprecated by the new index added. So, let's just stop duplicating the
> > information inside modules.builtin.alias.bin and just use the other
> > index.
> > ---
> >  libkmod/libkmod-module.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> > index 6f7747c..40d394a 100644
> > --- a/libkmod/libkmod-module.c
> > +++ b/libkmod/libkmod-module.c
> > @@ -576,13 +576,14 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
> >       err = kmod_lookup_alias_from_aliases_file(ctx, alias, list);
> >       CHECK_ERR_AND_FINISH(err, fail, list, finish);
> >
> > +     DBG(ctx, "lookup modules.builtin %s\n", alias);
> > +     err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> > +
>
> assigning to the err variable looks useless. It will be overwritten
> anyway.
>


we were supposed to have a CHECK_ERR_AND_FINISH(err, fail, list, finish);
here.

thanks
Lucas De Marchi

[PATCH v2] libkmod: Prefer builtin index over builtin.alias

[Lucas De Marchi]

The modules.builtin.alias.bin is way larger than the
modules.builtin.bin.  On a normal "distro kernel":

	21k modules.builtin.alias.bin
	11k modules.builtin.bin

From the kernel we get both modules.builtin and modules.builtin.modinfo.
depmod generates modules.builtin.bin and modules.builtin.alias.bin
from them respectively. modules.bultin is not going away: it's not
deprecated by the new index added. So, let's just stop duplicating the
information inside modules.builtin.alias.bin and just use the other
index.
---
 libkmod/libkmod-module.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 6f7747c..6423339 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -576,13 +576,15 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
 	err = kmod_lookup_alias_from_aliases_file(ctx, alias, list);
 	CHECK_ERR_AND_FINISH(err, fail, list, finish);
 
+	DBG(ctx, "lookup modules.builtin %s\n", alias);
+	err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
+	CHECK_ERR_AND_FINISH(err, fail, list, finish);
+
 	DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
 	err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
-	if (err == -ENOSYS) {
-		/* Optional index missing, try the old one */
-		DBG(ctx, "lookup modules.builtin %s\n", alias);
-		err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
-	}
+	/* Optional index missing, ignore */
+	if (err == -ENOSYS)
+		err = 0;
 	CHECK_ERR_AND_FINISH(err, fail, list, finish);
 
 
-- 
2.35.1

Re: [PATCH v2] libkmod: Prefer builtin index over builtin.alias

[Alexey Gladkov]

On Sat, Feb 12, 2022 at 09:43:35PM -1000, Lucas De Marchi wrote:
> The modules.builtin.alias.bin is way larger than the
> modules.builtin.bin.  On a normal "distro kernel":
> 
> 	21k modules.builtin.alias.bin
> 	11k modules.builtin.bin
> 
> >From the kernel we get both modules.builtin and modules.builtin.modinfo.
> depmod generates modules.builtin.bin and modules.builtin.alias.bin
> from them respectively. modules.bultin is not going away: it's not
> deprecated by the new index added. So, let's just stop duplicating the
> information inside modules.builtin.alias.bin and just use the other
> index.

The modules.builtin contains only module names. The modules.builtin.modinfo
contains full info about builtin modules including names.

I thought that if there is complete information about the modules, then
reading the index with only the names does not make sense. And only in the
absence of modules.builtin.modinfo, you need to fallback to the index
with the names.

> ---
>  libkmod/libkmod-module.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 6f7747c..6423339 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -576,13 +576,15 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
>  	err = kmod_lookup_alias_from_aliases_file(ctx, alias, list);
>  	CHECK_ERR_AND_FINISH(err, fail, list, finish);
>  
> +	DBG(ctx, "lookup modules.builtin %s\n", alias);
> +	err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> +	CHECK_ERR_AND_FINISH(err, fail, list, finish);
> +
>  	DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
>  	err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> -	if (err == -ENOSYS) {
> -		/* Optional index missing, try the old one */
> -		DBG(ctx, "lookup modules.builtin %s\n", alias);
> -		err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> -	}
> +	/* Optional index missing, ignore */
> +	if (err == -ENOSYS)
> +		err = 0;
>  	CHECK_ERR_AND_FINISH(err, fail, list, finish);

Yep. Looks good for me.

-- 
Rgrds, legion

Re: [PATCH v2] libkmod: Prefer builtin index over builtin.alias

[Lucas De Marchi]

On Sun, Feb 13, 2022 at 02:13:39PM +0100, Alexey Gladkov wrote:
>On Sat, Feb 12, 2022 at 09:43:35PM -1000, Lucas De Marchi wrote:
>> The modules.builtin.alias.bin is way larger than the
>> modules.builtin.bin.  On a normal "distro kernel":
>>
>> 	21k modules.builtin.alias.bin
>> 	11k modules.builtin.bin
>>
>> >From the kernel we get both modules.builtin and modules.builtin.modinfo.
>> depmod generates modules.builtin.bin and modules.builtin.alias.bin
>> from them respectively. modules.bultin is not going away: it's not
>> deprecated by the new index added. So, let's just stop duplicating the
>> information inside modules.builtin.alias.bin and just use the other
>> index.
>
>The modules.builtin contains only module names. The modules.builtin.modinfo
>contains full info about builtin modules including names.
>
>I thought that if there is complete information about the modules, then
>reading the index with only the names does not make sense. And only in the
>absence of modules.builtin.modinfo, you need to fallback to the index
>with the names.

yeah, but most of the time we really need only the module name, so we
can optimize for that. And since we are not getting rid of the other
index, we can simply use it first the same way that for modules we first
do lookup on lookup modules.dep and only later on modules.aliases.

>
>> ---
>>  libkmod/libkmod-module.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> index 6f7747c..6423339 100644
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -576,13 +576,15 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
>>  	err = kmod_lookup_alias_from_aliases_file(ctx, alias, list);
>>  	CHECK_ERR_AND_FINISH(err, fail, list, finish);
>>
>> +	DBG(ctx, "lookup modules.builtin %s\n", alias);
>> +	err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
>> +	CHECK_ERR_AND_FINISH(err, fail, list, finish);
>> +
>>  	DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
>>  	err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
>> -	if (err == -ENOSYS) {
>> -		/* Optional index missing, try the old one */
>> -		DBG(ctx, "lookup modules.builtin %s\n", alias);
>> -		err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
>> -	}
>> +	/* Optional index missing, ignore */
>> +	if (err == -ENOSYS)
>> +		err = 0;
>>  	CHECK_ERR_AND_FINISH(err, fail, list, finish);
>
>Yep. Looks good for me.

thanks
Lucas De Marchi

>
>-- 
>Rgrds, legion
>

Re: [PATCH v2] libkmod: Prefer builtin index over builtin.alias

[Alexey Gladkov]

On Mon, Feb 14, 2022 at 11:43:10PM -0800, Lucas De Marchi wrote:
> On Sun, Feb 13, 2022 at 02:13:39PM +0100, Alexey Gladkov wrote:
> > On Sat, Feb 12, 2022 at 09:43:35PM -1000, Lucas De Marchi wrote:
> > > The modules.builtin.alias.bin is way larger than the
> > > modules.builtin.bin.  On a normal "distro kernel":
> > > 
> > > 	21k modules.builtin.alias.bin
> > > 	11k modules.builtin.bin
> > > 
> > > >From the kernel we get both modules.builtin and modules.builtin.modinfo.
> > > depmod generates modules.builtin.bin and modules.builtin.alias.bin
> > > from them respectively. modules.bultin is not going away: it's not
> > > deprecated by the new index added. So, let's just stop duplicating the
> > > information inside modules.builtin.alias.bin and just use the other
> > > index.
> > 
> > The modules.builtin contains only module names. The modules.builtin.modinfo
> > contains full info about builtin modules including names.
> > 
> > I thought that if there is complete information about the modules, then
> > reading the index with only the names does not make sense. And only in the
> > absence of modules.builtin.modinfo, you need to fallback to the index
> > with the names.
> 
> yeah, but most of the time we really need only the module name, so we
> can optimize for that. And since we are not getting rid of the other
> index, we can simply use it first the same way that for modules we first
> do lookup on lookup modules.dep and only later on modules.aliases.

Yes and no. We can be sure that we don't need aliases. The argument passed
to utilities can be very similar to the name of a module:

$ modinfo fs-ext4
name:           ext4
filename:       (builtin)
softdep:        pre: crc32c
license:        GPL
file:           fs/ext4/ext4
description:    Fourth Extended Filesystem
author:         Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others
alias:          fs-ext4
alias:          ext3
alias:          fs-ext3

By the way, crc32c is also an alias:

$ modinfo crc32c
filename:       /lib/modules/5.14.0.61-centos-alt1.el9/kernel/arch/x86/crypto/crc32c-intel.ko
alias:          crypto-crc32c-intel
alias:          crc32c-intel
alias:          crypto-crc32c
alias:          crc32c
license:        GPL
description:    CRC32c (Castagnoli) optimization using Intel Hardware.
author:         Austin Zhang <austin.zhang@intel.com>, Kent Liu <kent.liu@intel.com>
rhelversion:    9.0
srcversion:     1F2B6A533C8243A4017180A
alias:          cpu:type:x86,ven*fam*mod*:feature:*0094*
depends:
retpoline:      Y
intree:         Y
name:           crc32c_intel
vermagic:       5.14.0.61-centos-alt1.el9 SMP preempt mod_unload modversions

This is because there are multiple implementations of crc32c. So, information
about alias of builtin modules is almost always needed.

> > 
> > > ---
> > >  libkmod/libkmod-module.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> > > index 6f7747c..6423339 100644
> > > --- a/libkmod/libkmod-module.c
> > > +++ b/libkmod/libkmod-module.c
> > > @@ -576,13 +576,15 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
> > >  	err = kmod_lookup_alias_from_aliases_file(ctx, alias, list);
> > >  	CHECK_ERR_AND_FINISH(err, fail, list, finish);
> > > 
> > > +	DBG(ctx, "lookup modules.builtin %s\n", alias);
> > > +	err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> > > +	CHECK_ERR_AND_FINISH(err, fail, list, finish);
> > > +
> > >  	DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
> > >  	err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> > > -	if (err == -ENOSYS) {
> > > -		/* Optional index missing, try the old one */
> > > -		DBG(ctx, "lookup modules.builtin %s\n", alias);
> > > -		err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> > > -	}
> > > +	/* Optional index missing, ignore */
> > > +	if (err == -ENOSYS)
> > > +		err = 0;
> > >  	CHECK_ERR_AND_FINISH(err, fail, list, finish);
> > 
> > Yep. Looks good for me.
> 
> thanks
> Lucas De Marchi
> 
> > 
> > -- 
> > Rgrds, legion
> > 
> 

-- 
Rgrds, legion

Re: [PATCH v2] libkmod: Prefer builtin index over builtin.alias

[Lucas De Marchi]

On Sat, Feb 19, 2022 at 01:40:42PM +0100, Alexey Gladkov wrote:
>On Mon, Feb 14, 2022 at 11:43:10PM -0800, Lucas De Marchi wrote:
>> On Sun, Feb 13, 2022 at 02:13:39PM +0100, Alexey Gladkov wrote:
>> > On Sat, Feb 12, 2022 at 09:43:35PM -1000, Lucas De Marchi wrote:
>> > > The modules.builtin.alias.bin is way larger than the
>> > > modules.builtin.bin.  On a normal "distro kernel":
>> > >
>> > > 	21k modules.builtin.alias.bin
>> > > 	11k modules.builtin.bin
>> > >
>> > > >From the kernel we get both modules.builtin and modules.builtin.modinfo.
>> > > depmod generates modules.builtin.bin and modules.builtin.alias.bin
>> > > from them respectively. modules.bultin is not going away: it's not
>> > > deprecated by the new index added. So, let's just stop duplicating the
>> > > information inside modules.builtin.alias.bin and just use the other
>> > > index.
>> >
>> > The modules.builtin contains only module names. The modules.builtin.modinfo
>> > contains full info about builtin modules including names.
>> >
>> > I thought that if there is complete information about the modules, then
>> > reading the index with only the names does not make sense. And only in the
>> > absence of modules.builtin.modinfo, you need to fallback to the index
>> > with the names.
>>
>> yeah, but most of the time we really need only the module name, so we
>> can optimize for that. And since we are not getting rid of the other
>> index, we can simply use it first the same way that for modules we first
>> do lookup on lookup modules.dep and only later on modules.aliases.
>
>Yes and no. We can be sure that we don't need aliases. The argument passed
>to utilities can be very similar to the name of a module:
>
>$ modinfo fs-ext4
>name:           ext4
>filename:       (builtin)
>softdep:        pre: crc32c
>license:        GPL
>file:           fs/ext4/ext4
>description:    Fourth Extended Filesystem
>author:         Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others
>alias:          fs-ext4
>alias:          ext3
>alias:          fs-ext3
>
>By the way, crc32c is also an alias:
>
>$ modinfo crc32c
>filename:       /lib/modules/5.14.0.61-centos-alt1.el9/kernel/arch/x86/crypto/crc32c-intel.ko
>alias:          crypto-crc32c-intel
>alias:          crc32c-intel
>alias:          crypto-crc32c
>alias:          crc32c
>license:        GPL
>description:    CRC32c (Castagnoli) optimization using Intel Hardware.
>author:         Austin Zhang <austin.zhang@intel.com>, Kent Liu <kent.liu@intel.com>
>rhelversion:    9.0
>srcversion:     1F2B6A533C8243A4017180A
>alias:          cpu:type:x86,ven*fam*mod*:feature:*0094*
>depends:
>retpoline:      Y
>intree:         Y
>name:           crc32c_intel
>vermagic:       5.14.0.61-centos-alt1.el9 SMP preempt mod_unload modversions
>
>This is because there are multiple implementations of crc32c. So, information
>about alias of builtin modules is almost always needed.

I was thinking about "optimization" related to libkmod/modprobe (which
is what matters during boot and for the systemd integration). Not for
modinfo which is only called by developers and auxiliary tools.
But then I measured it doing 50k random lookups. The speed up exists but
is under 1% and within the error margin, particularly because we need to
maintain the order doing the alias lookup first. So the main benefit is
really "we are not getting rid of the other index, so could as well just
not duplicate the info".

Another reason for not getting rid of the module is to be able to force
the lookup to be by module name, ignoring the alias. That was the reason
I sent this other series:
https://lore.kernel.org/linux-modules/Yg4DOfCUvIpDDBRd@bombadil.infradead.org/T/#t
so one can get the modinfo from the crc32 module, and not from its
aliases:

	$ modinfo --modname crc32
	module explicitly:
	name:           crc32
	filename:       (builtin)
	license:        GPL
	file:           lib/crc32
	description:    Various CRC32 calculations
	author:         Matt Domsch <Matt_Domsch@dell.com>

Lucas De Marchi