+ 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 <sgarz...@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 > > ><sgarz...@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.mikhalit...@canonical.com> > > >> >--- > > >> > drivers/vhost/vsock.c | 1 + > > >> > 1 file changed, 1 insertion(+) > > >> > > > >> >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > > >> >index 802153e23073..287ea8e480b5 100644 > > >> >--- a/drivers/vhost/vsock.c > > >> >+++ b/drivers/vhost/vsock.c > > >> >@@ -956,6 +956,7 @@ static void __exit vhost_vsock_exit(void) > > >> > > > >> > module_init(vhost_vsock_init); > > >> > module_exit(vhost_vsock_exit); > > >> >+MODULE_VERSION("0.0.1"); > > > > > >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 <mcg...@kernel.org> Please use linux-modules in the future as I'm not the only maintainer. Luis