add kobject to struct module

Hi,

A while ago we had talked about adding a kobject to struct module. By doing this we add support for module paramaters and other module info to be exported in sysfs. So here's a patch that does this that is against 2.6.0-test4 (it applies with some fuzz, sorry.)

This patch adds basic kobject support to struct module, and it creates a /sys/module directory which contains all of the individual modules. Each module currently exports only one file, the module refcount:

	$ tree /sys/module/
	/sys/module/
	|-- ehci_hcd
	|   `-- refcount
	|-- hid
	|   `-- refcount
	|-- parport
	|   `-- refcount
	|-- parport_pc
	|   `-- refcount
	|-- uhci_hcd
	|   `-- refcount
	`-- usbcore
	    `-- refcount

I used the kobject reference count to add to the module reference count to handle races if a user has a module owned sysfs file open, but this reference is not exported to userspace, as that just confuses the userspace tools a bunch (and I don't want to force people to upgrade module-init-tools this late in the development cycle...)

Rusty, any comments? If this looks sane, I'll work on adding the module_paramater support to the individual module directories.

thanks,

greg k-h

# Module: Add a kobject to struct module
#
# This gets us /sys/module to show all modules.
# Module attributes will happen next.

diff -Nru a/include/linux/module.h b/include/linux/module.h
@@ -16,6 +16,7 @@
 #include <linux/kmod.h>
 #include <linux/elf.h>
 #include <linux/stringify.h>
+#include <linux/kobject.h>
 #include <asm/local.h>
 
 #include <asm/module.h>

@@ -184,6 +185,8 @@
 
 struct module
 {
+	struct kobject	kobj;
+
 	enum module_state state;
 
 	/* Member of list of modules */

Ugh, that should read "2.6.0-test5"...

greg k-h

In message <[email protected]> you write:

I'd just started on the same thing, but I'll use yours as a bae.

I'm not sure if embedding the kobject in the module is the correct approach in this case, because we can't use the kobject refcount for modules because it's too slow. This cannot be fixed before 2.7 8(

Because kobject does not have a "struct module *owner", we can't simply add in the refcount. The module reference count is defined to never go from zero to one when the module is dying, which means callers must use try_module_get(). I grab the reference on read/write, which means opening the file won't hold the module, either.

Were you intending to put all the info currently in /proc/modules under sysfs? Makes sense I think. For the options you'll need a subdir to avoid name clashes.

Cheers!
Rusty.

Name: Modules in sysfs
Author: Greg KH <[email protected]>
Status: Tested on 2.6.0-test5

D: Modified by Rusty to allocate kobject and module separately. D:
D: This patch adds basic kobject support to struct module, and it creates a
D: /sys/module directory which contains all of the individual modules.
D: Each module currently exports only one file, the module refcount:
D: $ tree /sys/module/
D: /sys/module/
D: |-- ehci_hcd
D: | `-- refcount
D: |-- hid
D: | `-- refcount
D: |-- parport
D: | `-- refcount
D: |-- parport_pc
D: | `-- refcount
D: |-- uhci_hcd
D: | `-- refcount
D: `-- usbcore
D: `-- refcount

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .26542-linux-2.6.0-test5/include/linux/module.h .26542-linux-2.6.0-test5.updated/include/linux/module.h
@@ -16,6 +16,7 @@
 #include <linux/kmod.h>
 #include <linux/elf.h>
 #include <linux/stringify.h>
+#include <linux/kobject.h>
 #include <asm/local.h>
 
 #include <asm/module.h>

@@ -182,6 +183,12 @@ enum module_state
 	MODULE_STATE_GOING,
 };
 
+struct module_kobj
+{
+	struct kobject kobj;
+	struct module *mod;
+};
+
 struct module
 {
 	enum module_state state;

@@ -192,6 +199,9 @@ struct module
 	/* Unique handle for this module */
 	char name[MODULE_NAME_LEN];
 
+	/* Our presence in sysfs. */
+	struct module_kobj *mkobj;
+
 	/* Exported symbols */
 	const struct kernel_symbol *syms;
 	unsigned int num_syms;

--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

That's fine, I do not want to use the kobject refcount for modules. modules have "special" refcount issues that you've already solved. I don't want to go down that rathole :)

Um, I don't understand. There is no "struct module *owner in struct kobject. There is one in struct attribute, but I don't set it, so it doesn't matter for this usage.

read/write of what? The attribute? Sure, why not set the module attribute sysfs file to the module that way the reference count will be incremented if the sysfs file is opened.

I'm not trying to touch the module reference count logic here, besides adding the kobject reference count to the internal module count logic. I think I got it all correct and it worked for me :)

But in looking at your patch, I don't see why you want to separate the module from the kobject? What benefit does it have?

Hey, you're the maintainer, it's your call :)

Yes, I was going to add it, this patch was more of a "test" to see how receptive you were to it.

If you want, I can add the options and other info based off of this patch.

thanks,

greg k-h

In message <[email protected]> you write:

Your parser broke, I think 8)

Hmm, because there's one attribute: which module would own it? You're going to creation attributes per module later (for module parameters), so when you do that it might make sense to do this too.

The lifetimes are separate, each controlled by their own reference count. I *know* this will work even if someone holds a reference to the kobject (for some reason in the future) even as the module is removed.

More more! 8)

Thanks,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

kernel/module.c owns the attribute code. (The same code and attribute structure is reused for each object its exported for, so the owner field must be set to the owner of the code itself.)

Correct me if I'm wrong, but this sounds similar to the networking refcount problem. The reference on the containing object is the interesting one, as far as visibility goes. As long as its positive, the module is active.

The kobject refcount can still be used for the lifetime of the object. It can simply be pinned the entire time the module is active. When the module is deleted, the kobject can be unregistered somewhere around free_module(). It looks like you might want to split that up into two
parts: "Before unregistering the kobject" that removed it from lists, etc, and "In kobject release method", which would call module_free() (and probably free the args then too).

This way you should retain the same semantics and be able to use the kobject for controlling the lifetime (which will allow you to safely export attributes through sysfs).

Make sense?

Pat

In message <[email protected]> you write:

Right, then the current code is correct.

There are basically two choices: ensure that the reference count is taken using try_module_get() (kobject doesn't have an owner field, so it does not match this one), or ensure that an object isn't ever referenced after the module cleanup function is called.

In this context, that means that the module cleanup must pause until the reference count of the kobject hits zero, so it can be freed.

Implementation below.

BTW, The *real* answer IMHO is (this is 2.7 stuff:)

1) Adopt a faster, smaller implementation of alloc_percpu (this patch exists, needs some arch-dependent love for ia64).
2) Use it to generalize the current module reference count scheme to a "bigref_t" (I have a couple of these)
3) Use that in kobjects.
4) Decide that module removal is not as important as it was, and not all modules need be removable (at least in finite time).
5) Use the kobject reference count everywhere, including modules.

This would make everything faster, except for the case where someone is actually waiting for a refcount to hit zero: for long-lived objects like kobjects, this seems the right tradeoff.

Cheers!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Modules in sysfs
Author: Greg KH <[email protected]>
Status: Tested on 2.6.0-test5-bk1

D: This patch adds basic kobject support to struct module, and it creates a
D: /sys/module directory which contains all of the individual modules.
D: Each module currently exports only one file, the module refcount:
D: $ tree /sys/module/
D: /sys/module/
D: |-- ehci_hcd
D: | `-- refcount
D: |-- hid
D: | `-- refcount
D: |-- parport
D: | `-- refcount
D: |-- parport_pc
D: | `-- refcount
D: |-- uhci_hcd
D: | `-- refcount
D: `-- usbcore
D: `-- refcount
D:
D: Rusty: We ensure that the kobject embedded in the module has a shorter
D: lifetime than the module itself by waiting for its reference count
D: to reach zero before freeing the module structure.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .28943-linux-2.6.0-test5/include/linux/module.h .28943-linux-2.6.0-test5.updated/include/linux/module.h
@@ -16,6 +16,7 @@
 #include <linux/kmod.h>
 #include <linux/elf.h>
 #include <linux/stringify.h>
+#include <linux/kobject.h>
 #include <asm/local.h>
 
 #include <asm/module.h>

@@ -184,6 +185,8 @@ enum module_state
 
 struct module
 {
+	struct kobject	kobj;
+
 	enum module_state state;
 
 	/* Member of list of modules */

@@ -230,6 +233,9 @@ struct module
 	/* Am I GPL-compatible */
 	int license_gplok;
 
+	/* Who is waiting for us to be unloaded, or kobject to be unused. */
+	struct task_struct *waiter;
+
 #ifdef CONFIG_MODULE_UNLOAD
 	/* Reference counts */
 	struct module_ref ref[NR_CPUS];

@@ -237,9 +243,6 @@ struct module
 	/* What modules depend on me? */
 	struct list_head modules_which_use_me;
 
-	/* Who is waiting for us to be unloaded */
-	struct task_struct *waiter;
-
 	/* Destruction function. */
 	void (*exit)(void);
 #endif

Ah, nice catch on that bug. I like this implementation.

On a site note, can't you just use a "struct completion" to use for your waiting? Or do you need to do something special here?

Hm, I don't know if kobjects really need to get that heavy.

As more people use kobjects, I think we'll see some pretty short lifespans...

But yes, that's all 2.7 dreams :)

thanks,

greg k-h

In message <[email protected]> you write:

Hmm, *good* question. Think...

Ah, it's because when someone's waiting for the reference count to hit zero, we wake them *every* time we decrement. With the reference count spread across every cpu, it's the only way:

static inline void module_put(struct module *module)
{
if (module) {
unsigned int cpu = get_cpu(); local_dec(&module->ref[cpu].count);
/* Maybe they're waiting for us to drop reference? */ if (unlikely(!module_is_live(module)))
wake_up_process(module->waiter);
put_cpu();
}
}

This doesn't really fit with a completion, unfortunately.

I'm not sure either: really depends on kobject usage. I was thinking struct netdevice. The size for UP is the same, the size for SMP is ptr + sizeof(int) + sizeof(atomic_t)*NR_CPUs.

Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Ah, thanks for the explaination. Makes sense.

greg k-h

Ok, I'm digging up some old code here...

Problem is, this implementation dies a horrible death if you grab the reference to the sysfs file and then try to unload the module at the same time :(

So, here's a patch on top of your patch (full patch against 2.6.3 is below), that simply makes the module reference file be owned by the module that it is assigned to. Yes, this means that when you actually read the file value, the reference is 1 greater than when it is closed, but that's the breaks. It does solve the "grab the file and then try to unload the module" problem, as it simply prevents this from happening.

Comments?

I think this whole patch can be simpler now, as we don't have to do the kobject_wait type logic, but if you think we should go back to doing that, I'll try to go debug what's wrong with your original patch...

thanks,

greg k-h

-------
Incremental patch:

diff -Nru a/include/linux/module.h b/include/linux/module.h
@@ -192,6 +192,7 @@
 struct module
 {
 	struct kobject	kobj;
+	struct module_attribute *mod_refcount;
 
 	enum module_state state;
 

diff -Nru a/kernel/module.c b/kernel/module.c
@@ -1130,7 +1130,7 @@
 	return sprintf(buf, "%d\n", module_refcount(mod));
 }
 
-static struct module_attribute mod_refcount = {
+static struct module_attribute mod_refcount_template = {
 	.attr = {.name = "refcount", .mode = S_IRUGO},
 	.show = show_mod_refcount,
 };

@@ -1148,22 +1148,33 @@
 {
 	int retval;
 
+	mod->mod_refcount = kmalloc(sizeof(struct module_attribute), GFP_KERNEL);
+	if (!mod->mod_refcount)
+		return -ENOMEM;
+	memcpy(mod->mod_refcount, &mod_refcount_template, sizeof(struct module_attribute));
+	mod->mod_refcount->attr.owner = mod;
+	
 	retval = kobject_set_name(&mod->kobj, mod->name);
 	if (retval < 0)
-		return retval;
+		goto error;
 	kobj_set_kset_s(mod, module_subsys);
 	retval = kobject_register(&mod->kobj);
 	if (retval)
-		return retval;
-	retval = sysfs_create_file(&mod->kobj, &mod_refcount.attr);
+		goto error;
+	retval = sysfs_create_file(&mod->kobj, &mod->mod_refcount->attr);
 	if (retval < 0)
 		remove_kobject_wait(mod);
 	return retval;
+
+error:
+	kfree(mod->mod_refcount);
+	return retval;
 }
 
 static void mod_kobject_remove(struct module *mod)
 {
-	sysfs_remove_file(&mod->kobj, &mod_refcount.attr);
+	sysfs_remove_file(&mod->kobj, &mod->mod_refcount->attr);
+	kfree(mod->mod_refcount);
 	remove_kobject_wait(mod);
 }
 

------------------

Full patch against a clean 2.6.3:

@@ -16,6 +16,7 @@
 #include <linux/kmod.h>
 #include <linux/elf.h>
 #include <linux/stringify.h>
+#include <linux/kobject.h>
 #include <asm/local.h>
 
 #include <asm/module.h>

@@ -190,6 +191,9 @@
 
 struct module
 {
+	struct kobject	kobj;
+	struct module_attribute *mod_refcount;
+
 	enum module_state state;
 
 	/* Member of list of modules */

@@ -236,15 +240,15 @@
 	/* Am I GPL-compatible */
 	int license_gplok;
 
+	/* Who is waiting for us to be unloaded, or kobject to be unused. */
+	struct task_struct *waiter;
+
 #ifdef CONFIG_MODULE_UNLOAD
 	/* Reference counts */
 	struct module_ref ref[NR_CPUS];
 
 	/* What modules depend on me? */
 	struct list_head modules_which_use_me;
-
-	/* Who is waiting for us to be unloaded */
-	struct task_struct *waiter;
 
 	/* Destruction function. */
 	void (*exit)(void);

I've rewritten your rewrite again 8)

This patch actually implements module_param in sysfs for modules. You can see my tests in crypto_null().

What's missing is inbuilt modules. I think /sys/modules/ne2k/debug should exist whether ne2k is built-in or modular, for example. Since the only thing we have is a kparam with "ne2k.debug" as its name, there will have to be some kludgery here, but we can end up with something OK.

Cheers,
Rusty.

Anyone who quotes me in their signature is an idiot -- Rusty Russell

Heh, ok, time to stop this rewriting...

This looks good. I made some _very_ minor tweaks, and have checked it into my driver tree, it will show up in the next -mm release. I've included the patch below if you want to verify it.

I agree, I'll work on that next. I have support for exporting the version, author, and license through the directory working, but I think I'll hold off on it for a while (if at all, exporting the license all of the time is pretty dumb...) But people have expressed interest in having the version exported to make it easier for bug-info-generating scripts to pick up (distros specifically.)

thanks for putting up with the slow pace of this patch :)

greg k-h

# Add modules to sysfs
#
# This patch adds basic kobject support to struct module, and it creates a

# /sys/module directory which contains all of the individual modules.  Each
# module currently exports the refcount (if they are unloadable) and any
# module paramaters that are marked exportable in sysfs.
# 
# Was written by me and Rusty over and over many times during the past 6 months.

diff -Nru a/include/linux/module.h b/include/linux/module.h
@@ -16,6 +16,8 @@
 #include <linux/kmod.h>
 #include <linux/elf.h>
 #include <linux/stringify.h>
+#include <linux/kobject.h>
+#include <linux/moduleparam.h>
 #include <asm/local.h>
 
 #include <asm/module.h>

@@ -207,6 +209,23 @@
 	MODULE_STATE_GOING,
 };
 
+/* sysfs stuff */
+struct module_attribute
+{
+	struct attribute attr;
+	struct kernel_param *param;
+};
+
+struct module_kobject
+{
+	/* Everyone should have one of these. */
+	struct kobject kobj;
+
+	/* We always have refcnt, we may have others from module_param(). */
+	unsigned int num_attributes;
+	struct module_attribute attr[0];
+};
+
 struct module
 {
 	enum module_state state;

@@ -217,6 +236,9 @@
 	/* Unique handle for this module */
 	char name[MODULE_NAME_LEN];
 
+	/* Sysfs stuff. */
+	struct module_kobject *mkobj;
+
 	/* Exported symbols */
 	const struct kernel_symbol *syms;
 	unsigned int num_syms;

@@ -267,6 +289,9 @@
 
 	/* Destruction function. */
 	void (*exit)(void);
+
+	/* Fake kernel param for refcnt. */
+	struct kernel_param refcnt_param;
 #endif
 
 #ifdef CONFIG_KALLSYMS

diff -Nru a/include/linux/moduleparam.h b/include/linux/moduleparam.h
@@ -50,7 +50,7 @@
    not there, read bits mean it's readable, write bits mean it's
    writable. */
 #define __module_param_call(prefix, name, set, get, arg, perm)		\
-	static char __param_str_##name[] __initdata = prefix #name;	\
+	static char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param const __param_##name			\
 	__attribute_used__						\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \

@@ -71,7 +71,7 @@
 
 /* Actually copy string: maxlen param is usually sizeof(string). */
 #define module_param_string(name, string, len, perm)			\
-	static struct kparam_string __param_string_##name __initdata	\
+	static struct kparam_string __param_string_##name		\
 		= { len, string };					\
 	module_param_call(name, param_set_copystring, param_get_charp,	\
 		   &__param_string_##name, perm)

diff -Nru a/kernel/params.c b/kernel/params.c
@@ -161,7 +161,7 @@
 									\
 		if (!val) return -EINVAL;				\
 		l = strtolfn(val, &endp, 0);				\
-		if (endp == val || *endp || ((type)l != l))		\
+		if (endp == val || ((type)l != l))			\
 			return -EINVAL;					\
 		*((type *)kp->arg) = l;					\
 		return 0;						\

Ok, let's try again. :)

Why are you detaching the kobject from struct module?
In my patch I accounted for the kobject's reference count in the module reference count (just not the count exported to userspace, as to not break the userspace tools.) So if a user has a module sysfs file open (like the "refcount" file), the module reference count is incremented and the module is not allowed to be unloaded until that count drops. This removes any race condition with the kobject being in use when the module structure is freed.

Does that make sense?

The attribute "refcount" is "owned" by the module itself. The kobject count is incremented if the file is opened by the sysfs core, thus preventing the module from being able to be unloaded.

The same thing will happen for module paramaters. They are owned by the module structure as well.

It all "just works" :)

But my patch prevented that from ever happening. If someone grabbed the kobject, the module could not be unloaded. That fixes all kinds of races.

But whose patch to build on, mine, or your version? :)

thanks,

greg k-h

In message <[email protected]> you write:

Sorry, my bad. This is related to another bug: you unregistered the kobject before checking the reference count. This is bad, because the remove can fail, and you don't re-register the kobject. Even if we re-register, now there's a spurious failure as the kobject vanishes for a moment and reappears. And let's not think about what happens if trying to re-register the kobject fails 8(

So I think we really do want to unregister the kobject as part of the cleanup, which makes things a little more complicated: my immediately previous patch which should do what we want. If it's still not clear, then I'm obviously doing a really crappy job of explaining...

Thanks,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Please excuse my short-sightedness, but I think the following points haven't been thought deeply enough:

- modules which use their parameters on initialisation only once. (eg, 3c59x "full_duplex" parameter)

- Also, what about module parameters which modules aren't expecting to change beneath themselves? (eg, all the watchdog modules)

- Are we opening the floodgates for another round of races and driver updates?

Russell King ([email protected])	http://www.arm.linux.org.uk/personal/
Linux kernel maintainer of:
  2.6 ARM Linux   - http://www.arm.linux.org.uk/
  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
  2.6 Serial core

If you set the "perm" portion of the module_param() macro to 0, then the sysfs file will not be created. That will cause all of the old modules that use the MODULE_PARAM() macro to not have things change for them, as they will not be expecting it.

To quote from include/linux/moduleparam.h:
/* This is the fundamental function for registering boot/module
parameters. perm sets the visibility in driverfs: 000 means it's not there, read bits mean it's readable, write bits mean it's writable. */

Sound ok with you?

thanks,

greg k-h

Any chance to make it always visible and read-only by default with the option of making it writable?

Exposing the module options would be very helpful.

Also showing its read/write in the sysfs directory listing would be great. (if it doesn't already do that).

Any chance the parameter defaults (if they're not hard coded...) could be exposed even if they're not given to the module on the command line? (wish list...)

I don't know. Doesn't matter to me.

Look at the permission bits :)

That's probably just a modinfo hack.

thanks,

greg k-h

In message <[email protected]> you write:

Nope. The author specifies exactly what they want, no default. It's just safer this way: see RMK's concerns about what would happen if we did it to unsuspecting module authors...

See include/linux/moduleparam.h, especially the module_param() macro.

They should be there. It would be nice to have some way of telling which ones were modified, so that a userspace util could save just those ones on shutdown, for example. I can't think of an obvious way of doing this though (mtime > epoch maybe?).

Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

In message <[email protected]> you write:

Well, obviously not publically enough, at least 8(.

To clarify: this is for new-style module params, which explicitly control their own visibility with the perm arg.

One more reason for module authors to switch 8)

Hope that helps,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.