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@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>---
> drivers/vhost/vsock.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index 802153e23073..287ea8e480b5 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -956,6 +956,7 @@ static void __exit vhost_vsock_exit(void)
>
> module_init(vhost_vsock_init);
> module_exit(vhost_vsock_exit);
>+MODULE_VERSION("0.0.1");
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
>
