Message-ID: <CAEivzxdP+7q9vDk-0V8tPuCo1mFw92jVx0u3B8jkyYKv8sLcdA@mail.gmail.com>
Date: Mon, 30 Sep 2024 19:03:52 +0200
From: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: 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, mcgrof@...nel.org
Subject: Re: [PATCH v2] vhost/vsock: specify module version

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.

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

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

+cc Luis Chamberlain <mcgrof@...nel.org>

>
> Your use case makes sense to me, so that we could try something like
> that, but obviously it requires more work I think.

I personally am pretty happy to do more work on the generic side if
it's really valuable
for other use cases and folks support the idea.

My first intention was to make a quick and easy fix but it turns out
that there are some
drawbacks which I have not seen initially.

>
> Again, I don't want to block this patch, but I'd like to see if there's
> a better way than this hack :-)

Yeah, I understand. Thanks a lot for reacting to this patch. I
appreciate it a lot!

Kind regards,
Alex

>
> Thanks,
> Stefano
>
> >
> >Then I found "module: show version information for built-in modules in sysfs":
> >https://github.com/torvalds/linux/commit/e94965ed5beb23c6fabf7ed31f625e66d7ff28de
> >and it inspired me to make this minimalistic change.
> >
> >>
> >> In particular the interesting thing is from nfp, where
> >> `MODULE_VERSION(UTS_RELEASE);` was added with this commit:
> >>
> >> 1a5e8e350005 ("nfp: populate MODULE_VERSION")
> >>
> >> And then removed completely with this commit:
> >>
> >> b4f37219813f ("net/nfp: Update driver to use global kernel version")
> >>
> >> CCing Jakub since he was involved, so maybe he can give us some
> >> pointers.
> >
> >Kind regards,
> >Alex
> >
> >>
> >> Thanks,
> >> Stefano
> >>
> >> > MODULE_LICENSE("GPL v2");
> >> > MODULE_AUTHOR("Asias He");
> >> > MODULE_DESCRIPTION("vhost transport for vsock ");
> >> >--
> >> >2.34.1
> >> >
> >>
> >
>

Powered by blists - more mailing lists