Re: [PATCH v2] vhost/vsock: specify module version
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 > > > > > > > > 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
Re: [PATCH v2] vhost/vsock: specify module version
On Thu, Oct 03, 2024 at 12:48:22PM -0700, Luis Chamberlain wrote:
+ linux-modu...@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 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
> > 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 root0 Sep 29 19:00 .
> >> >drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> >> >drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> >> >drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> >> >drwxr-xr-x 100 root root0 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
> >> >---
> >> > 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: driver
Re: [PATCH v2] vhost/vsock: specify module version
+ linux-modu...@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
> 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
> > > 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 root0 Sep 29 19:00 .
> > >> >drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> > >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> > >> >drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> > >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> > >> >drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> > >> >drwxr-xr-x 100 root root0 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
> > >> >
> > >> >---
> > >> > 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
Please use linux-modules in the future as I'm not the only maintainer.
Luis
Re: [PATCH v2] vhost/vsock: specify module version
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 > > > > > 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
> > >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
On Mon, Sep 30, 2024 at 5:43 PM Stefano Garzarella 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
> > 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 root0 Sep 29 19:00 .
> >> >drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> >> >drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> >> >-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> >> >drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> >> >drwxr-xr-x 100 root root0 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
> >> >---
> >> > 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
>
> 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
>
Re: [PATCH v2] vhost/vsock: specify module version
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
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 root0 Sep 29 19:00 .
>drwxr-xr-x 337 root root0 Sep 29 18:59 ..
>-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
>drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
>-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
>drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
>drwxr-xr-x 100 root root0 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
>---
> 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
On Mon, Sep 30, 2024 at 4:27 PM Stefano Garzarella 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 root0 Sep 29 19:00 .
> >drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> >-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> >drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> >-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> >drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> >drwxr-xr-x 100 root root0 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
> >---
> > 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
On Mon, Sep 30, 2024 at 4:05 PM Michael S. Tsirkin 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 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 root0 Sep 29 19:00 .
> > > > drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> > > > -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> > > > drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> > > > -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> > > > drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> > > > drwxr-xr-x 100 root root0 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 is loaded as a module OR
> > configured
> > as a built-in then /sys/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/
> > 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 root0 Sep 29 19:00 .
> > drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> > -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> > drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> > -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> > drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> > drwxr-xr-x 100 root root0 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
> >
> >
> > Does this sound fair enough?
> >
> > Kind re
Re: [PATCH v2] vhost/vsock: specify module version
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 root0 Sep 29 19:00 .
drwxr-xr-x 337 root root0 Sep 29 18:59 ..
-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
drwxr-xr-x 100 root root0 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
---
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
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 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 root0 Sep 29 19:00 .
> > > drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> > > drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> > > drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> > > drwxr-xr-x 100 root root0 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 is loaded as a module OR
> configured
> as a built-in then /sys/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/
> 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 root0 Sep 29 19:00 .
> drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> drwxr-xr-x 100 root root0 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
>
>
> Does this sound fair enough?
>
> Kind regards,
> Alex
Looks good, thanks!
> >
> >
> >
> > > Signed-off-by: Alexander Mikhalitsyn
> > > ---
> > > 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 10064
Re: [PATCH v2] vhost/vsock: specify module version
On Sun, Sep 29, 2024 at 9:03 PM Michael S. Tsirkin 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 root0 Sep 29 19:00 .
> > drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> > -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> > drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> > -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> > drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> > drwxr-xr-x 100 root root0 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 is loaded as a module OR configured
as a built-in then /sys/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/
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 root0 Sep 29 19:00 .
drwxr-xr-x 337 root root0 Sep 29 18:59 ..
-r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
-r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
drwxr-xr-x 100 root root0 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
Does this sound fair enough?
Kind regards,
Alex
>
>
>
> > Signed-off-by: Alexander Mikhalitsyn
> > ---
> > 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
Re: [PATCH v2] vhost/vsock: specify module version
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 root0 Sep 29 19:00 .
> drwxr-xr-x 337 root root0 Sep 29 18:59 ..
> -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> drwxr-xr-x 2 root root0 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 root0 Sep 29 20:05 notes
> -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> drwxr-xr-x 2 root root0 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 root0 Sep 26 15:59 .
> drwxr-xr-x 100 root root0 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
> ---
> 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
