Message-ID: <pyocjxtoqd6qtbkszq77odcakfh7arn3bmj72xb5elso3rksvy@tdjcfqddf3sd>
Date: Thu, 3 Oct 2024 15:55:23 -0500
From: Lucas De Marchi <lucas.demarchi@...el.com>
To: Luis Chamberlain <mcgrof@...nel.org>
CC: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>, "Stefano
 Garzarella" <sgarzare@...hat.com>, <kuba@...nel.org>, <stefanha@...hat.com>,
	"Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>,
	Eugenio Pérez <eperezma@...hat.com>, <kvm@...r.kernel.org>,
	<virtualization@...ts.linux.dev>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v2] vhost/vsock: specify module version

On Thu, Oct 03, 2024 at 12:48:22PM -0700, Luis Chamberlain wrote:
>+ linux-modules@...r.kernel.org + 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@...hat.com> 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@...hat.com> 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@...onical.com>
>> > >> >---
>> > >> > 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@...fusion.mobi>
	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@...nel.org>
>
>Please use linux-modules in the future as I'm not the only maintainer.
>
>  Luis

Powered by blists - more mailing lists