[PATCH v2] vhost/vsock: specify module version

[PATCH v2] vhost/vsock: specify module version

[Alexander Mikhalitsyn]

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

Re: [PATCH v2] vhost/vsock: specify module version

[Michael S. Tsirkin]

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

Sorry, what I'd like to see is an explanation which userspace
is broken without this change, and whether this patch fixes is.



> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@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

Re: [PATCH v2] vhost/vsock: specify module version

[Aleksandr Mikhalitsyn]

On Sun, Sep 29, 2024 at 9:03 PM Michael S. Tsirkin <mst@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.mikhalitsyn@canonical.com>
============

Does this sound fair enough?

Kind regards,
Alex

>
>
>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@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
>

Re: [PATCH v2] vhost/vsock: specify module version

[Michael S. Tsirkin]

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@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.mikhalitsyn@canonical.com>
> ============
> 
> Does this sound fair enough?
> 
> Kind regards,
> Alex


Looks good, thanks!

> >
> >
> >
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@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
> >

Re: [PATCH v2] vhost/vsock: specify module version

[Stefano Garzarella]

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@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");

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.

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.

Thanks,
Stefano

> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Asias He");
> MODULE_DESCRIPTION("vhost transport for vsock ");
>-- 
>2.34.1
>

Re: [PATCH v2] vhost/vsock: specify module version

[Aleksandr Mikhalitsyn]

On Mon, Sep 30, 2024 at 4:05 PM Michael S. Tsirkin <mst@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 <mst@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.mikhalitsyn@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.mikhalitsyn@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
> > >
>

Re: [PATCH v2] vhost/vsock: specify module version

[Aleksandr Mikhalitsyn]

On Mon, Sep 30, 2024 at 4:27 PM Stefano Garzarella <sgarzare@redhat.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@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");

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.

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.

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

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

Re: [PATCH v2] vhost/vsock: specify module version

[Stefano Garzarella]

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@redhat.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@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");
>
>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?

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.

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

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

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

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

Re: [PATCH v2] vhost/vsock: specify module version

[Aleksandr Mikhalitsyn]

On Mon, Sep 30, 2024 at 5:43 PM Stefano Garzarella <sgarzare@redhat.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@redhat.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@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");
> >
> >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@kernel.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
> >> >
> >>
> >
>

Re: [PATCH v2] vhost/vsock: specify module version

[Andrew Lunn]

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

As you said, all over vhost modules say version 0.0.1 for years. But
the kernel around these modules has had many changes. So 0.0.1 tells
you nothing useful. When a user reports a problem using vhost_vsock
version 0.0.1 your first question is going to be, what kernel version
from which distribution?

If the information is useless, let just remove it.

I would not base a kAPI around this. Isn't there a system call you can
perform and get an EOPNOTSUPP, ENODEV, EMORECOFFEE?

Also, at least in networking and probably in other subsystems,
performing an operation is often needed to trigger the module to
load. So it not being listed in /sys/modules does not mean the module
is missing, it could be its just not been needed until now.

Take a step back, what is your real use case here? Describe it and
maybe we can think of a better kAPI.

	Andrew

Re: [PATCH v2] vhost/vsock: specify module version

[Jakub Kicinski]

On Mon, 30 Sep 2024 19:03:52 +0200 Aleksandr Mikhalitsyn wrote:
> > 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@kernel.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.

IMHO a generic solution would be much better. I can't help but feel
like exposing an arbitrary version to get the module to show up in 
sysfs is a hack.

IIUC the list of built in modules is available in
/lib/modules/*/modules.builtin, the user space can't read that?

Re: [PATCH v2] vhost/vsock: specify module version

[Luis Chamberlain]

+ linux-modules@vger.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@redhat.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@redhat.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@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");
> > >
> > >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.

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

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

> Probably, yes. We can ask Luis Chamberlain's opinion on this one.
> 
> +cc Luis Chamberlain <mcgrof@kernel.org>

Please use linux-modules in the future as I'm not the only maintainer.

  Luis

Re: [PATCH v2] vhost/vsock: specify module version

[Lucas De Marchi]

On Thu, Oct 03, 2024 at 12:48:22PM -0700, Luis Chamberlain wrote:
>+ linux-modules@vger.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@redhat.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@redhat.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@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");
>> > >
>> > >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@profusion.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@kernel.org>
>
>Please use linux-modules in the future as I'm not the only maintainer.
>
>  Luis

Re: [PATCH v2] vhost/vsock: specify module version

[Michael S. Tsirkin]

On Wed, Oct 02, 2024 at 07:16:02AM -0700, Jakub Kicinski wrote:
> On Mon, 30 Sep 2024 19:03:52 +0200 Aleksandr Mikhalitsyn wrote:
> > > 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@kernel.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.
> 
> IMHO a generic solution would be much better. I can't help but feel
> like exposing an arbitrary version to get the module to show up in 
> sysfs is a hack.
> 
> IIUC the list of built in modules is available in
> /lib/modules/*/modules.builtin, the user space can't read that?


So what are we doing about this? Aleksandr?

-- 
MST