On Sun, Sep 29, 2024 at 9:03 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Sun, Sep 29, 2024 at 08:21:03PM +0200, 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.
>
> And that's expected.
>
> > 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
>
Hi Michael,
> Sorry, what I'd like to see is an explanation which userspace
> is broken without this change, and whether this patch fixes is.
Ok, let me try to write a proper commit message in this thread. I'll
send a v3 once we agree on it (don't want to spam busy
kvm developers with my one-liner fix in 10 different revisions :-) ).
============
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. We already have userspace consumers [1], [2]
who rely on the
assumption that if <any_linux_kernel_module> is loaded as a module OR configured
as a built-in then /sys/module/<any_linux_kernel_module> exists. While
this assumption
works well in most cases it is wrong in general. For a built-in module
X you get a /sys/module/<X>
only if the module declares MODULE_VERSION or if the module has any
parameter(s) declared.
Let's just declare MODULE_VERSION("0.0.1") for vhost_vsock to make
/sys/module/vhost_vsock
to exist in all possible configurations (loadable module or built-in).
Version 0.0.1 is chosen to align
with all other modules in drivers/vhost.
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
Link: https://github.com/canonical/lxd/blob/ef33aea98aec9778499e96302f2605882d8249d7/lxd/instance/drivers/driver_qemu.go#L8568
[1]
Link: https://github.com/lxc/incus/blob/cbebce1dcd5f15887967058c8f6fec27cf0da2a2/internal/server/instance/drivers/driver_qemu.go#L8723
[2]
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@xxxxxxxxxxxxx>
============
Does this sound fair enough?
Kind regards,
Alex
>
>
>
> > 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");
> > MODULE_LICENSE("GPL v2");
> > MODULE_AUTHOR("Asias He");
> > MODULE_DESCRIPTION("vhost transport for vsock ");
> > --
> > 2.34.1
>
