[PATCH] Kmod optimization

[PATCH] Kmod optimization

[tyler]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

Hi,

the request_mod functions try to load automatically a module by running
a user mode process helper (modprobe).

The user process is launched even if the module is already loaded. I
think it would be better to test if the module is already loaded.

-- 
tyler
tyler@agat.net

[-- Attachment #2: patch_kmod_2.6.16.5 --]
[-- Type: text/plain, Size: 2817 bytes --]

diff -uprN -X linux-2.6.16.5/Documentation/dontdiff linux-2.6.16.5/include/linux/module.h linux-2.6.16.5-tyler/include/linux/module.h
--- linux-2.6.16.5/include/linux/module.h	2006-04-13 19:53:01.000000000 +0200
+++ linux-2.6.16.5-tyler/include/linux/module.h	2006-04-13 19:17:21.000000000 +0200
@@ -338,6 +338,7 @@ struct module *module_get_kallsym(unsign
 unsigned long module_kallsyms_lookup_name(const char *name);
 
 int is_exported(const char *name, const struct module *mod);
+int is_loaded(const char *module_name);
 
 extern void __module_put_and_exit(struct module *mod, long code)
 	__attribute__((noreturn));
diff -uprN -X linux-2.6.16.5/Documentation/dontdiff linux-2.6.16.5/kernel/kmod.c linux-2.6.16.5-tyler/kernel/kmod.c
--- linux-2.6.16.5/kernel/kmod.c	2006-04-13 19:53:02.000000000 +0200
+++ linux-2.6.16.5-tyler/kernel/kmod.c	2006-04-13 19:19:32.000000000 +0200
@@ -37,6 +37,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <asm/uaccess.h>
+#include <linux/spinlock.h>
 
 extern int max_threads;
 
@@ -48,6 +49,7 @@ static struct workqueue_struct *khelper_
 	modprobe_path is set via /proc/sys.
 */
 char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
+extern spinlock_t modlist_lock;
 
 /**
  * request_module - try to load a kernel module
@@ -77,6 +79,7 @@ int request_module(const char *fmt, ...)
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	unsigned long flags;
 
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
@@ -84,6 +87,15 @@ int request_module(const char *fmt, ...)
 	if (ret >= MODULE_NAME_LEN)
 		return -ENAMETOOLONG;
 
+	/* We don't to load the module if it's already loaded */
+	spin_lock_irqsave(&modlist_lock, flags);
+	if (is_loaded(module_name)) {
+		return -EEXIST;
+	}
+	spin_unlock_irqrestore(&modlist_lock, flags);
+
+
+
 	/* If modprobe needs a service that is in a module, we get a recursive
 	 * loop.  Limit the number of running kmod threads to max_threads/2 or
 	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
diff -uprN -X linux-2.6.16.5/Documentation/dontdiff linux-2.6.16.5/kernel/module.c linux-2.6.16.5-tyler/kernel/module.c
--- linux-2.6.16.5/kernel/module.c	2006-04-13 19:54:37.000000000 +0200
+++ linux-2.6.16.5-tyler/kernel/module.c	2006-04-13 19:21:11.000000000 +0200
@@ -212,6 +212,19 @@ static struct module *find_module(const 
 	return NULL;
 }
 
+/* Test if a module is loaded : must hold module_mutex */
+int is_loaded(const char *module_name);
+{
+	struct module *mod = find_module(module_name);
+
+	if (!mod) {
+		return 1;
+	}
+
+	return 0;
+}
+
+
 #ifdef CONFIG_SMP
 /* Number of blocks used and allocated. */
 static unsigned int pcpu_num_used, pcpu_num_allocated;

Re: [PATCH] Kmod optimization

[Randy.Dunlap]

On Thu, 13 Apr 2006 20:03:45 +0200 tyler@agat.net wrote:

> Hi,
> 
> the request_mod functions try to load automatically a module by running
> a user mode process helper (modprobe).
> 
> The user process is launched even if the module is already loaded. I
> think it would be better to test if the module is already loaded.

Please try not to use attachments: it makes it more difficult
to review and comment on code (so I'll paste it here).


+	/* We don't to load the module if it's already loaded */
+	spin_lock_irqsave(&modlist_lock, flags);
+	if (is_loaded(module_name)) {
+		return -EEXIST;
+	}
+	spin_unlock_irqrestore(&modlist_lock, flags);

Need to do spin_unlock_irqrestore() even if is_loaded() is true.

+/* Test if a module is loaded : must hold module_mutex */
+int is_loaded(const char *module_name);
+{
+	struct module *mod = find_module(module_name);
+
+	if (!mod) {
+		return 1;
+	}

Don't use braces when not needed.
Why not make this function inline and put it into a header file?

---
~Randy

Re: [PATCH] Kmod optimization

[Greg KH]

On Thu, Apr 13, 2006 at 08:03:45PM +0200, tyler@agat.net wrote:
> Hi,
> 
> the request_mod functions try to load automatically a module by running
> a user mode process helper (modprobe).
> 
> The user process is launched even if the module is already loaded. I
> think it would be better to test if the module is already loaded.

Does this cause a problem somehow?  request_mod is called _very_
infrequently from a normal kernel these days, so I really don't think
this is necessary.

thanks,

greg k-h

Re: [PATCH] Kmod optimization

[tyler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 3581 bytes --]

On Thu, Apr 13, 2006 at 11:24:01AM -0700, Greg KH wrote:
> On Thu, Apr 13, 2006 at 08:03:45PM +0200, tyler@agat.net wrote:
> > Hi,
> > 
> > the request_mod functions try to load automatically a module by running
> > a user mode process helper (modprobe).
> > 
> > The user process is launched even if the module is already loaded. I
> > think it would be better to test if the module is already loaded.
> 
> Does this cause a problem somehow?  request_mod is called _very_
> infrequently from a normal kernel these days, so I really don't think
> this is necessary.

Yes I agree it _should_ be very infrequently called but it _will_ be very
infrequently called just if the user space configuration is done properly.

I personnaly think we shouldn't trust the configuration and the way the
different modules are loaded.

Well anyway, I've corrected the patch regarding the advices of randy.

diff -uprN -X linux-2.6.16.5/Documentation/dontdiff linux-2.6.16.5/include/linux/module.h linux-2.6.16.5-tyler/include/linux/module.h
--- linux-2.6.16.5/include/linux/module.h	2006-04-13 20:03:16.000000000 +0200
+++ linux-2.6.16.5-tyler/include/linux/module.h	2006-04-13 20:24:39.000000000 +0200
@@ -339,6 +339,19 @@ unsigned long module_kallsyms_lookup_nam
 
 int is_exported(const char *name, const struct module *mod);
 
+/* Test if a module is loaded : must hold module_mutex */
+inline int is_loaded(const char *module_name)
+{
+	struct module *mod = find_module(module_name);
+
+	if (!mod)
+		return 1;
+
+	return 0;
+}
+
+
+
 extern void __module_put_and_exit(struct module *mod, long code)
 	__attribute__((noreturn));
 #define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE, code);
diff -uprN -X linux-2.6.16.5/Documentation/dontdiff linux-2.6.16.5/kernel/kmod.c linux-2.6.16.5-tyler/kernel/kmod.c
--- linux-2.6.16.5/kernel/kmod.c	2006-04-13 20:03:16.000000000 +0200
+++ linux-2.6.16.5-tyler/kernel/kmod.c	2006-04-13 20:22:28.000000000 +0200
@@ -37,6 +37,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <asm/uaccess.h>
+#include <linux/spinlock.h>
 
 extern int max_threads;
 
@@ -48,6 +49,7 @@ static struct workqueue_struct *khelper_
 	modprobe_path is set via /proc/sys.
 */
 char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
+extern spinlock_t modlist_lock;
 
 /**
  * request_module - try to load a kernel module
@@ -77,6 +79,7 @@ int request_module(const char *fmt, ...)
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	unsigned long flags;
 
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
@@ -84,6 +87,16 @@ int request_module(const char *fmt, ...)
 	if (ret >= MODULE_NAME_LEN)
 		return -ENAMETOOLONG;
 
+	/* We don't to load the module if it's already loaded */
+	spin_lock_irqsave(&modlist_lock, flags);
+	if (is_loaded(module_name)) {
+		spin_unlock_irqrestore(&modlist_lock, flags);
+		return -EEXIST;
+	}
+	spin_unlock_irqrestore(&modlist_lock, flags);
+
+
+
 	/* If modprobe needs a service that is in a module, we get a recursive
 	 * loop.  Limit the number of running kmod threads to max_threads/2 or
 	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method


	

	
		
___________________________________________________________________________ 
Faites de Yahoo! votre page d'accueil sur le web pour retrouver directement vos services préférés : vérifiez vos nouveaux mails, lancez vos recherches et suivez l'actualité en temps réel. 
Rendez-vous sur http://fr.yahoo.com/set

Re: [PATCH] Kmod optimization

[Greg KH]

On Thu, Apr 13, 2006 at 08:36:17PM +0200, tyler@agat.net wrote:
> On Thu, Apr 13, 2006 at 11:24:01AM -0700, Greg KH wrote:
> > On Thu, Apr 13, 2006 at 08:03:45PM +0200, tyler@agat.net wrote:
> > > Hi,
> > > 
> > > the request_mod functions try to load automatically a module by running
> > > a user mode process helper (modprobe).
> > > 
> > > The user process is launched even if the module is already loaded. I
> > > think it would be better to test if the module is already loaded.
> > 
> > Does this cause a problem somehow?  request_mod is called _very_
> > infrequently from a normal kernel these days, so I really don't think
> > this is necessary.
> 
> Yes I agree it _should_ be very infrequently called but it _will_ be very
> infrequently called just if the user space configuration is done properly.

What do you mean by this?  Almost all 2.6 distros use udev today, which
prevents this code from ever getting called.  So odds are, you are
optimising something that no one will ever use :)

> I personnaly think we shouldn't trust the configuration and the way the
> different modules are loaded.

What way different modules are loaded?  What problem are you referring
to here?

thanks,

greg k-h

Re: [PATCH] Kmod optimization

[Jan Engelhardt]

+/* Test if a module is loaded : must hold module_mutex */
+int is_loaded(const char *module_name);
+{
+       struct module *mod = find_module(module_name);
+
+       if (!mod) {
+               return 1;
+       }
+
+       return 0;
+}
+
+


>Don't use braces when not needed.
>Why not make this function inline and put it into a header file?

static inline int is_loaded(const char *module_name) {
    return find_module(module_name) != NULL;
}

Cheers. In that case, I think it can even be "hand-inlined" into the callers.


Jan Engelhardt
-- 

Re: [PATCH] Kmod optimization

[tyler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 2068 bytes --]

On Thu, Apr 13, 2006 at 11:50:14AM -0700, Greg KH wrote:
> On Thu, Apr 13, 2006 at 08:36:17PM +0200, tyler@agat.net wrote:
> > On Thu, Apr 13, 2006 at 11:24:01AM -0700, Greg KH wrote:
> > > On Thu, Apr 13, 2006 at 08:03:45PM +0200, tyler@agat.net wrote:
> > > > Hi,
> > > > 
> > > > the request_mod functions try to load automatically a module by running
> > > > a user mode process helper (modprobe).
> > > > 
> > > > The user process is launched even if the module is already loaded. I
> > > > think it would be better to test if the module is already loaded.
> > > 
> > > Does this cause a problem somehow?  request_mod is called _very_
> > > infrequently from a normal kernel these days, so I really don't think
> > > this is necessary.
> > 
> > Yes I agree it _should_ be very infrequently called but it _will_ be very
> > infrequently called just if the user space configuration is done properly.
> 
> What do you mean by this?  Almost all 2.6 distros use udev today, which
> prevents this code from ever getting called.  So odds are, you are
> optimising something that no one will ever use :)
Well perhaps I don't understand the mechanism :) But let's take an
example.
On all kernels (even recent), if the module smbfs is loaded, it's not
handled by udev and request_module could be called.

Let"s take another example to see to illustrate why I think
it depends on the user configuration :
module A depends on module B

if we have a script which do "insmod moduleA.ko ; insmod moduleB.ko",
there will be a call to request_module.

if the script is "insmod moduleB.ko ; insmod moduleA.ko", request_mode
is not called.

I know the first script is really idiot :)

This is what I was thinking about in my previous mail.

-- 
tyler
tyler@agat.net


	

	
		
___________________________________________________________________________ 
Faites de Yahoo! votre page d'accueil sur le web pour retrouver directement vos services préférés : vérifiez vos nouveaux mails, lancez vos recherches et suivez l'actualité en temps réel. 
Rendez-vous sur http://fr.yahoo.com/set

Re: [PATCH] Kmod optimization

[Paulo Marques]

tyler@agat.net wrote:
>
> +/* Test if a module is loaded : must hold module_mutex */
> +inline int is_loaded(const char *module_name)
> +{
> +	struct module *mod = find_module(module_name);
> +
> +	if (!mod)
> +		return 1;

FWIW, this logic seems reversed....

-- 
Paulo Marques - www.grupopie.com

Pointy-Haired Boss: I don't see anything that could stand in our way.
            Dilbert: Sanity? Reality? The laws of physics?

Re: [PATCH] Kmod optimization

[tyler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 3026 bytes --]

On Thu, Apr 13, 2006 at 08:10:35PM +0100, Paulo Marques wrote:
> tyler@agat.net wrote:
> >
> >+/* Test if a module is loaded : must hold module_mutex */
> >+inline int is_loaded(const char *module_name)
> >+{
> >+	struct module *mod = find_module(module_name);
> >+
> >+	if (!mod)
> >+		return 1;

For sure it is. Sorry, my mistake :)

diff -uprN -X linux-2.6.16.5/Documentation/dontdiff linux-2.6.16.5/include/linux/module.h linux-2.6.16.5-tyler/include/linux/module.h
--- linux-2.6.16.5/include/linux/module.h	2006-04-13 20:03:16.000000000 +0200
+++ linux-2.6.16.5-tyler/include/linux/module.h	2006-04-13 20:24:39.000000000 +0200
@@ -339,6 +339,19 @@ unsigned long module_kallsyms_lookup_nam
 
 int is_exported(const char *name, const struct module *mod);
 
+/* Test if a module is loaded : must hold module_mutex */
+inline int is_loaded(const char *module_name)
+{
+	struct module *mod = find_module(module_name);
+
+	if (!mod)
+		return 0;
+
+	return 1;
+}
+
+
+
 extern void __module_put_and_exit(struct module *mod, long code)
 	__attribute__((noreturn));
 #define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE, code);
diff -uprN -X linux-2.6.16.5/Documentation/dontdiff linux-2.6.16.5/kernel/kmod.c linux-2.6.16.5-tyler/kernel/kmod.c
--- linux-2.6.16.5/kernel/kmod.c	2006-04-13 20:03:16.000000000 +0200
+++ linux-2.6.16.5-tyler/kernel/kmod.c	2006-04-13 20:22:28.000000000 +0200
@@ -37,6 +37,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <asm/uaccess.h>
+#include <linux/spinlock.h>
 
 extern int max_threads;
 
@@ -48,6 +49,7 @@ static struct workqueue_struct *khelper_
 	modprobe_path is set via /proc/sys.
 */
 char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
+extern spinlock_t modlist_lock;
 
 /**
  * request_module - try to load a kernel module
@@ -77,6 +79,7 @@ int request_module(const char *fmt, ...)
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	unsigned long flags;
 
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
@@ -84,6 +87,16 @@ int request_module(const char *fmt, ...)
 	if (ret >= MODULE_NAME_LEN)
 		return -ENAMETOOLONG;
 
+	/* We don't to load the module if it's already loaded */
+	spin_lock_irqsave(&modlist_lock, flags);
+	if (is_loaded(module_name)) {
+		spin_unlock_irqrestore(&modlist_lock, flags);
+		return -EEXIST;
+	}
+	spin_unlock_irqrestore(&modlist_lock, flags);
+
+
+
 	/* If modprobe needs a service that is in a module, we get a recursive
 	 * loop.  Limit the number of running kmod threads to max_threads/2 or
 	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method


	

	
		
___________________________________________________________________________ 
Faites de Yahoo! votre page d'accueil sur le web pour retrouver directement vos services préférés : vérifiez vos nouveaux mails, lancez vos recherches et suivez l'actualité en temps réel. 
Rendez-vous sur http://fr.yahoo.com/set

Re: [PATCH] Kmod optimization

[Roman Zippel]

Hi,

On Thu, 13 Apr 2006, tyler@agat.net wrote:

> Well perhaps I don't understand the mechanism :) But let's take an
> example.
> On all kernels (even recent), if the module smbfs is loaded, it's not
> handled by udev and request_module could be called.

No, it can't. If the smbfs is loaded, get_fs_type() will find it and won't 
even try to load it.
Do you a real example, where this is a problem?

bye, Roman

Re: [PATCH] Kmod optimization

[Greg KH]

On Thu, Apr 13, 2006 at 09:04:12PM +0200, tyler@agat.net wrote:
> On Thu, Apr 13, 2006 at 11:50:14AM -0700, Greg KH wrote:
> > On Thu, Apr 13, 2006 at 08:36:17PM +0200, tyler@agat.net wrote:
> > > On Thu, Apr 13, 2006 at 11:24:01AM -0700, Greg KH wrote:
> > > > On Thu, Apr 13, 2006 at 08:03:45PM +0200, tyler@agat.net wrote:
> > > > > Hi,
> > > > > 
> > > > > the request_mod functions try to load automatically a module by running
> > > > > a user mode process helper (modprobe).
> > > > > 
> > > > > The user process is launched even if the module is already loaded. I
> > > > > think it would be better to test if the module is already loaded.
> > > > 
> > > > Does this cause a problem somehow?  request_mod is called _very_
> > > > infrequently from a normal kernel these days, so I really don't think
> > > > this is necessary.
> > > 
> > > Yes I agree it _should_ be very infrequently called but it _will_ be very
> > > infrequently called just if the user space configuration is done properly.
> > 
> > What do you mean by this?  Almost all 2.6 distros use udev today, which
> > prevents this code from ever getting called.  So odds are, you are
> > optimising something that no one will ever use :)
> Well perhaps I don't understand the mechanism :) But let's take an
> example.
> On all kernels (even recent), if the module smbfs is loaded, it's not
> handled by udev and request_module could be called.
> 
> Let"s take another example to see to illustrate why I think
> it depends on the user configuration :
> module A depends on module B
> 
> if we have a script which do "insmod moduleA.ko ; insmod moduleB.ko",
> there will be a call to request_module.

No, that script will fail.  Try it out :)

The kernel will not call out to try to resolve the symbols, that's up to
userspace to handle.  Hint, try running 'modprobe moduleA.ko' instead,
it will handle the dependancies correctly.

I still don't see where this is really needed...

thanks,

greg k-h

Re: [PATCH] Kmod optimization

[tyler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 1261 bytes --]

On Thu, Apr 13, 2006 at 04:13:30PM -0700, Greg KH wrote:
> The kernel will not call out to try to resolve the symbols, that's up to
> userspace to handle.  Hint, try running 'modprobe moduleA.ko' instead,
> it will handle the dependancies correctly.
> 
> I still don't see where this is really needed...
> 
> thanks,
> 
> greg k-h

I meant, the purpose of request_module is to handle dependencies from the
kernel : if we need a functionnality, we request it. It's the definition
of dependency.

But I understood the mechanism by reading some request_module examples.
In fact, we test the avalaibility of the functionnality before calling
the request_module function.

So the case described in my previous message would normally never
happen.

if (!functionnality_not_avalaible) {
	module_request(functionnality)

	if (!functionnality_not_avalaible)
	    goto error;
}

Sorry, so the discussion is closed : forget it :)

-- 
tyler
tyler@agat.net


	

	
		
___________________________________________________________________________ 
Faites de Yahoo! votre page d'accueil sur le web pour retrouver directement vos services préférés : vérifiez vos nouveaux mails, lancez vos recherches et suivez l'actualité en temps réel. 
Rendez-vous sur http://fr.yahoo.com/set