On Mon, Sep 30, 2024 at 4:05 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Mon, Sep 30, 2024 at 02:28:30PM +0200, Aleksandr Mikhalitsyn wrote:
> > 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
>
>
> Looks good, thanks!
Thanks, Michael! And I'm sorry for not being clear in my commit
messages from the beginning of our discussion ;-)
Then I'll send v3 a bit later as I see that Stefano reacted to this
proposal too, will see how it goes :-)
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
> > >
>
