| Message ID | 20240929182103.21882-1-aleksandr.mikhalitsyn@canonical.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2] vhost/vsock: specify module version | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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 >
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 > >
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 >
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 > > > >
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 > > >
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 >> > >> >
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 > >> > > >> > > >
> > >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
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?
+ 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
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
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?
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 ");
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(+)