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

[Luis Chamberlain]

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

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

> 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