Re: [PATCH v2] vhost/vsock: specify module version

From: Lucas De Marchi
Date: Thu Oct 03 2024 - 16:56:00 EST

On Thu, Oct 03, 2024 at 12:48:22PM -0700, Luis Chamberlain wrote:
+ linux-modules@xxxxxxxxxxxxxxx + Lucas

On Mon, Sep 30, 2024 at 07:03:52PM +0200, Aleksandr Mikhalitsyn wrote:
On Mon, Sep 30, 2024 at 5:43 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
>
> Hi Aleksandr,
>
> On Mon, Sep 30, 2024 at 04:43:36PM GMT, Aleksandr Mikhalitsyn wrote:
> >On Mon, Sep 30, 2024 at 4:27 PM Stefano Garzarella
> ><sgarzare@xxxxxxxxxx> wrote:
> >>
> >> On Sun, Sep 29, 2024 at 08:21:03PM GMT, Alexander Mikhalitsyn wrote:
> >> >Add an explicit MODULE_VERSION("0.0.1") specification for the vhost_vsock module.
> >> >
> >> >It is useful because it allows userspace to check if vhost_vsock is there when it is
> >> >configured as a built-in.
> >> >
> >> >This is what we have *without* this change and when vhost_vsock is
> >> >configured
> >> >as a module and loaded:
> >> >
> >> >$ ls -la /sys/module/vhost_vsock
> >> >total 0
> >> >drwxr-xr-x 5 root root 0 Sep 29 19:00 .
> >> >drwxr-xr-x 337 root root 0 Sep 29 18:59 ..
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> >> >drwxr-xr-x 2 root root 0 Sep 29 20:05 holders
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 initsize
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 initstate
> >> >drwxr-xr-x 2 root root 0 Sep 29 20:05 notes
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> >> >drwxr-xr-x 2 root root 0 Sep 29 20:05 sections
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 srcversion
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 taint
> >> >--w------- 1 root root 4096 Sep 29 19:00 uevent
> >> >
> >> >When vhost_vsock is configured as a built-in there is *no* /sys/module/vhost_vsock directory at all.
> >> >And this looks like an inconsistency.
> >> >
> >> >With this change, when vhost_vsock is configured as a built-in we get:
> >> >$ ls -la /sys/module/vhost_vsock/
> >> >total 0
> >> >drwxr-xr-x 2 root root 0 Sep 26 15:59 .
> >> >drwxr-xr-x 100 root root 0 Sep 26 15:59 ..
> >> >--w------- 1 root root 4096 Sep 26 15:59 uevent
> >> >-r--r--r-- 1 root root 4096 Sep 26 15:59 version
> >> >
> >> >Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@xxxxxxxxxxxxx>
> >> >---
> >> > drivers/vhost/vsock.c | 1 +
> >> > 1 file changed, 1 insertion(+)
> >> >
> >> >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> >> >index 802153e23073..287ea8e480b5 100644
> >> >--- a/drivers/vhost/vsock.c
> >> >+++ b/drivers/vhost/vsock.c
> >> >@@ -956,6 +956,7 @@ static void __exit vhost_vsock_exit(void)
> >> >
> >> > module_init(vhost_vsock_init);
> >> > module_exit(vhost_vsock_exit);
> >> >+MODULE_VERSION("0.0.1");
> >
> >Hi Stefano,
> >
> >>
> >> I was looking at other commits to see how versioning is handled in order
> >> to make sense (e.g. using the same version of the kernel), and I saw
> >> many commits that are removing MODULE_VERSION because they say it
> >> doesn't make sense in in-tree modules.
> >
> >Yeah, I agree absolutely. I guess that's why all vhost modules have
> >had version 0.0.1 for years now
> >and there is no reason to increment version numbers at all.
>
> Yeah, I see.
>
> >
> >My proposal is not about version itself, having MODULE_VERSION
> >specified is a hack which
> >makes a built-in module appear in /sys/modules/ directory.
>
> Hmm, should we base a kind of UAPI on a hack?

Good question ;-)

>
> I don't want to block this change, but I just wonder why many modules
> are removing MODULE_VERSION and we are adding it instead.

Yep, that's a good point. I didn't know that other modules started to
remove MODULE_VERSION.

MODULE_VERSION was a stupid idea and there is no real value to it.
I agree folks should just remove its use and we remove it.

agreed - that should really be gone and shouldn't be used for this
purpose.


> >I spent some time reading the code in kernel/params.c and
> >kernel/module/sysfs.c to figure out
> >why there is no /sys/module/vhost_vsock directory when vhost_vsock is
> >built-in. And figured out the
> >precise conditions which must be satisfied to have a module listed in
> >/sys/module.
> >
> >To be more precise, built-in module X appears in /sys/module/X if one
> >of two conditions are met:
> >- module has MODULE_VERSION declared
> >- module has any parameter declared

I knew about the parameters dir. I didn't know about MODULE_VERSION.

>
> At this point my question is, should we solve the problem higher and
> show all the modules in /sys/modules, either way?

Because if you have no attribute to list why would you? The thing you
are trying to ask is different: "is this a module built-in" and for that we
have userpsace solution already suggested: /lib/modules/*/modules.builtin


yeah, that's right. The kernel build system provides that information
and depmod creates and index for lookup. There's both
/lib/modules/*/modules.builtin and modules.builtin.modinfo, which allows
this e.g.:

$ modinfo simpledrm
name: simpledrm
filename: (builtin)
license: GPL v2
file: drivers/gpu/drm/tiny/simpledrm
description: DRM driver for simple-framebuffer platform devices

For the libkmod API, you can use kmod_module_get_initstate()

To be honest I was also surprised long ago and thought that it would be
a good idea to have an empty dir for builtin modules... This is what I
added in kmod's TODO file:

commit 5e690c5cbdebca91998599a2b19542802ae0f7b0
Author: Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx>
Date: Fri Dec 16 02:02:58 2011 -0200

TODO: add new tasks and notes to future development

...

+Things to be added removed in kernel (check what is really needed):
+===================================================================
+
+* list of currently loaded modules

and later expanded on the idea:

* list of currently loaded modules
+ - readdir() in /sys/modules: dirs without a 'initstate' file mean the
+ modules is builtin.

I still think it would be "a nice to have", however there was never a
pressuring need for implementing it.

If we are going to have something like this, then please don't do that
via a dummy module parameter or module version.

Lucas De Marchi


Probably, yes. We can ask Luis Chamberlain's opinion on this one.

+cc Luis Chamberlain <mcgrof@xxxxxxxxxx>

Please use linux-modules in the future as I'm not the only maintainer.

Luis