On Mon, Sep 30, 2024 at 4:05 PM Michael S. Tsirkin <m...@redhat.com> 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 <m...@redhat.com> 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.mikhalit...@canonical.com> > > ============ > > > > 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.mikhalit...@canonical.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"); > > > > MODULE_LICENSE("GPL v2"); > > > > MODULE_AUTHOR("Asias He"); > > > > MODULE_DESCRIPTION("vhost transport for vsock "); > > > > -- > > > > 2.34.1 > > > >