[PATCH] Limit number of concurrent hotplug processes

[PATCH] Limit number of concurrent hotplug processes

[Hannes Reinecke]

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

Hi all,

the attached patch limits the number of concurrent hotplug processes.
Main idea behind it is that currently each call to call_usermodehelper 
will result in an execve() of "/sbin/hotplug", without any check whether 
enough resources are available for successful execution. This leads to 
hotplug being stuck and in worst cases to machines being unable to boot.

This check cannot be implemented in userspace as the resources are 
already taken by the time any resource check can be done; for the same 
reason any 'slim' programs as /sbin/hotplug will only delay the problem.

Any comments/suggestions welcome; otherwise please apply.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

[-- Attachment #2: kmod_limit_processes.patch --]
[-- Type: text/x-patch, Size: 6065 bytes --]

--- linux-2.6.8-rc1/kernel/kmod.c.orig	2004-07-14 15:55:41.000000000 +0200
+++ linux-2.6.8-rc1/kernel/kmod.c	2004-07-20 16:40:25.424425628 +0200
@@ -41,6 +41,9 @@
 extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
+static wait_queue_head_t kmod_waitq;
+static atomic_t kmod_concurrent = ATOMIC_INIT(50);
+static int kmod_max_concurrent = 50;
 
 #ifdef CONFIG_KMOD
 
@@ -67,16 +70,12 @@ int request_module(const char *fmt, ...)
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
-	unsigned int max_modprobes;
 	int ret;
 	char *argv[] = { modprobe_path, "-q", "--", module_name, NULL };
 	static char *envp[] = { "HOME=/",
 				"TERM=linux",
 				"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
 				NULL };
-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
-	static int kmod_loop_msg;
 
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
@@ -95,21 +94,11 @@ int request_module(const char *fmt, ...)
 	 *
 	 * "trace the ppid" is simple, but will fail if someone's
 	 * parent exits.  I think this is as good as it gets. --RR
+	 *
+	 * Resource checking is now implemented in 
+	 * call_usermodehelper --hare
 	 */
-	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
-	atomic_inc(&kmod_concurrent);
-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg++ < 5)
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-		atomic_dec(&kmod_concurrent);
-		return -ENOMEM;
-	}
-
 	ret = call_usermodehelper(modprobe_path, argv, envp, 1);
-	atomic_dec(&kmod_concurrent);
 	return ret;
 }
 EXPORT_SYMBOL(request_module);
@@ -181,6 +170,21 @@ static int wait_for_helper(void *data)
 	struct subprocess_info *sub_info = data;
 	pid_t pid;
 	struct k_sigaction sa;
+	int flags = SIGCHLD, retval, wait = sub_info->wait;
+
+	if (atomic_dec_and_test(&kmod_concurrent)) {
+		/* 
+		 * We have exceeded the maximum number of
+		 * concurrent kmod invocations. Delay this process
+		 * until enough resources are available again.
+		 */
+		printk(KERN_INFO "Delaying kmod event (current %d, max %d)\n",
+		       atomic_read(&kmod_concurrent), kmod_max_concurrent);
+		wait_event(kmod_waitq,
+			   atomic_read(&kmod_concurrent) > 0);
+	}
+	printk(KERN_INFO "Exec kmod event (current %d, max %d)\n",
+	       atomic_read(&kmod_concurrent), kmod_max_concurrent);
 
 	/* Install a handler: if SIGCLD isn't handled sys_wait4 won't
 	 * populate the status, but will return -ECHILD. */
@@ -190,10 +194,29 @@ static int wait_for_helper(void *data)
 	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
 	allow_signal(SIGCHLD);
 
-	pid = kernel_thread(____call_usermodehelper, sub_info, SIGCHLD);
+	if (!wait) {
+		/* CLONE_VFORK: wait until the usermode helper has execve'd
+		 * successfully We need the data structures to stay around
+		 * until that is done.  */
+		flags |= CLONE_VFORK;
+	}
+
+	pid = kernel_thread(____call_usermodehelper, sub_info, flags);
 	if (pid < 0) {
 		sub_info->retval = pid;
-	} else {
+		complete(sub_info->complete);
+		return 0;
+	}
+	/* 
+	 * usermodehelper started successfully
+	 * We always block for the child to exit as we want to
+	 * keep track of the number of currently running processes.
+	 * The only difference between asynchonous and synchronous
+	 * is that we notify the caller immediately for the former case
+	 * and delay the notification until the child exited for the 
+	 * latter.
+	 */
+	if (wait) {
 		/*
 		 * Normally it is bogus to call wait4() from in-kernel because
 		 * wait4() wants to write the exit code to a userspace address.
@@ -205,8 +228,23 @@ static int wait_for_helper(void *data)
 		 */
 		sys_wait4(pid, (int __user *) &sub_info->retval, 0, NULL);
 	}
-
+	/* Return the correct return value (if any) and notify caller */
 	complete(sub_info->complete);
+	
+	/* sub_info is not valid anymore, we need to use local variables */
+	if (!wait) {
+		/*
+		 * Wait for the child; we should only decrement the
+		 * counter after the child has indeed exited
+		 */
+		sys_wait4(pid, (int __user *) &retval, 0, NULL);
+	}
+	/* Increment the counter, we're done here */
+	atomic_inc(&kmod_concurrent);
+
+	/* Wake any sleeping kmod invocations */
+	wake_up(&kmod_waitq);
+
 	return 0;
 }
 
@@ -216,21 +254,13 @@ static void __call_usermodehelper(void *
 	struct subprocess_info *sub_info = data;
 	pid_t pid;
 
-	/* CLONE_VFORK: wait until the usermode helper has execve'd
-	 * successfully We need the data structures to stay around
-	 * until that is done.  */
-	if (sub_info->wait)
-		pid = kernel_thread(wait_for_helper, sub_info,
-				    CLONE_FS | CLONE_FILES | SIGCHLD);
-	else
-		pid = kernel_thread(____call_usermodehelper, sub_info,
-				    CLONE_VFORK | SIGCHLD);
+	pid = kernel_thread(wait_for_helper, sub_info,
+			    CLONE_FS | CLONE_FILES | SIGCHLD);
 
 	if (pid < 0) {
 		sub_info->retval = pid;
 		complete(sub_info->complete);
-	} else if (!sub_info->wait)
-		complete(sub_info->complete);
+	}
 }
 
 /**
@@ -276,4 +306,32 @@ void __init usermodehelper_init(void)
 {
 	khelper_wq = create_singlethread_workqueue("khelper");
 	BUG_ON(!khelper_wq);
+	init_waitqueue_head(&kmod_waitq);
+
+	/*
+	 * Limit the max number of concurrent processes
+	 * to something sane; 10 - max_threads/2 seems ok.
+	 */
+	if (kmod_max_concurrent > max_threads/2)
+		kmod_max_concurrent = max_threads/2;
+	if (kmod_max_concurrent < 10)
+		kmod_max_concurrent = 10;
+
+	printk(KERN_INFO "kmod: max %d concurrent processes\n",
+	       kmod_max_concurrent);
+	atomic_set(&kmod_concurrent,kmod_max_concurrent);
 }
+
+/*
+ * We want to restrict the maximum number of concurrent processes
+ * to max_threads / 2; however, at this time max_threads is not 
+ * yet initialised. So we will do the real check in usermodehelper_init().
+ */
+static int __init kmod_setup(char *s)
+{
+	get_option(&s, &kmod_max_concurrent);
+
+	return 1;
+}
+__setup("kmod_max=", kmod_setup);
+

Re: [PATCH] Limit number of concurrent hotplug processes

[Andrew Morton]

Hannes Reinecke <hare@suse.de> wrote:
>
> the attached patch limits the number of concurrent hotplug processes.
>  Main idea behind it is that currently each call to call_usermodehelper 
>  will result in an execve() of "/sbin/hotplug", without any check whether 
>  enough resources are available for successful execution. This leads to 
>  hotplug being stuck and in worst cases to machines being unable to boot.
> 
>  This check cannot be implemented in userspace as the resources are 
>  already taken by the time any resource check can be done; for the same 
>  reason any 'slim' programs as /sbin/hotplug will only delay the problem.

hm, it's a bit sad that this happens.  Are you able to tell us more about
what is causing such an explosion of module probes?

>  Any comments/suggestions welcome; otherwise please apply.

I suggest you just use a semaphore, initialised to a suitable value:


static struct semaphore foo = __SEMAPHORE_INITIALIZER(foo, 50);


{
	...
	down(&foo);
	...
	up(&foo);
	...
}



-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Christian Borntraeger]

Andrew Morton wrote:
> Hannes Reinecke <hare@suse.de> wrote:
> > the attached patch limits the number of concurrent hotplug processes.

> hm, it's a bit sad that this happens.  Are you able to tell us more about
> what is causing such an explosion of module probes?

I dont know exactly which scenario Hannes has seen, but its quite simple to 
trigger this scenario with almost any s390/zSeries.

Using the Hardware Management Console or z/VM you are able to hotplug 
(deactive/activate/attach/detach) almost every channel path. As a channel 
path can connect a big bunch of devices lots of machine checks are 
triggered, which causes lots of hotplugs. The same amount of machine checks 
could happen if a hardware failure happens. 

Some month ago I played around with a diet version of  hotplug. This program 
was fast and small enough to make my scenario work properly. Nevertheless, 
as hannes already stated this will only delay the problem. 

cheers

Christian



-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Hannes Reinecke]

Andrew Morton wrote:
> Hannes Reinecke <hare@suse.de> wrote:
> 
>>the attached patch limits the number of concurrent hotplug processes.
>> Main idea behind it is that currently each call to call_usermodehelper 
>> will result in an execve() of "/sbin/hotplug", without any check whether 
>> enough resources are available for successful execution. This leads to 
>> hotplug being stuck and in worst cases to machines being unable to boot.
>>
>> This check cannot be implemented in userspace as the resources are 
>> already taken by the time any resource check can be done; for the same 
>> reason any 'slim' programs as /sbin/hotplug will only delay the problem.
> 
> 
> hm, it's a bit sad that this happens.  Are you able to tell us more about
> what is causing such an explosion of module probes?
> 
As Christian Borntraeger already said, it's not so much an explosion of 
module probes but rather the triggering of quite a lot of events. 
Imagine loading scsi_debug with 512 devices or more ...

> 
>> Any comments/suggestions welcome; otherwise please apply.
> 
> 
> I suggest you just use a semaphore, initialised to a suitable value:
> 
> 
> static struct semaphore foo = __SEMAPHORE_INITIALIZER(foo, 50);
> 
> 
> {
> 	...
> 	down(&foo);
> 	...
> 	up(&foo);
> 	...
> }
> 
Hmm; looks good, but: It's not possible to reliably change the maximum 
number of processes on the fly.

The trivial way of course it when the waitqueue is empty and no 
processes are holding the semaphore. But it's quite non-obvious how this 
should work if processes are already holding the semaphore.
We would need to wait for those processes to finish, setting the length 
of the queue to 0 (to disallow any other process from grabbing the 
semaphore), and atomically set the queue length to the new value.
Apart from the fact that we would need a worker thread for that 
(otherwise the calling process might block indefinitely), there is no 
guarantee that the queue ever will become empty, as hotplug processes 
might be generated at any time.

Or is there an easier way?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&opÌk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Andrew Morton]

Hannes Reinecke <hare@suse.de> wrote:
>
> > 
>  >> Any comments/suggestions welcome; otherwise please apply.
>  > 
>  > 
>  > I suggest you just use a semaphore, initialised to a suitable value:
>  > 
>  > 
>  > static struct semaphore foo = __SEMAPHORE_INITIALIZER(foo, 50);
>  > 
>  > 
>  > {
>  > 	...
>  > 	down(&foo);
>  > 	...
>  > 	up(&foo);
>  > 	...
>  > }
>  > 
>  Hmm; looks good, but: It's not possible to reliably change the maximum 
>  number of processes on the fly.
> 
>  The trivial way of course it when the waitqueue is empty and no 
>  processes are holding the semaphore. But it's quite non-obvious how this 
>  should work if processes are already holding the semaphore.
>  We would need to wait for those processes to finish, setting the length 
>  of the queue to 0 (to disallow any other process from grabbing the 
>  semaphore), and atomically set the queue length to the new value.
>  Apart from the fact that we would need a worker thread for that 
>  (otherwise the calling process might block indefinitely), there is no 
>  guarantee that the queue ever will become empty, as hotplug processes 
>  might be generated at any time.
> 
>  Or is there an easier way?

Well if you want to increase the maximum number by ten you do:

	for (i = 0; i < 10; i++)
		up(&foo);

and similarly for decreasing the limit.  That will involve doing down()s,
which will automatically wait for the current number of threads to fall to
the desired level.

But I don't really see a need to tune this on the fly - probably the worse
problem occurs during bootup when the operator cannot perform tuning.

So a __setup parameter seems to be the best way of providing tunability. 
Initialise the semaphore in usermodehelper_init().


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Hannes Reinecke]

Andrew Morton wrote:
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> >> Any comments/suggestions welcome; otherwise please apply.
>> > 
>> > 
>> > I suggest you just use a semaphore, initialised to a suitable value:
>> > 
>> > 
>> > static struct semaphore foo = __SEMAPHORE_INITIALIZER(foo, 50);
>> > 
>> > 
>> > {
>> > 	...
>> > 	down(&foo);
>> > 	...
>> > 	up(&foo);
>> > 	...
>> > }
>> > 
>> Hmm; looks good, but: It's not possible to reliably change the maximum 
>> number of processes on the fly.
>>
>> The trivial way of course it when the waitqueue is empty and no 
>> processes are holding the semaphore. But it's quite non-obvious how this 
>> should work if processes are already holding the semaphore.
>> We would need to wait for those processes to finish, setting the length 
>> of the queue to 0 (to disallow any other process from grabbing the 
>> semaphore), and atomically set the queue length to the new value.
>> Apart from the fact that we would need a worker thread for that 
>> (otherwise the calling process might block indefinitely), there is no 
>> guarantee that the queue ever will become empty, as hotplug processes 
>> might be generated at any time.
>>
>> Or is there an easier way?
> 
> 
> Well if you want to increase the maximum number by ten you do:
> 
> 	for (i = 0; i < 10; i++)
> 		up(&foo);
> 
Indeed. That will work nicely.

> and similarly for decreasing the limit.  That will involve doing down()s,
> which will automatically wait for the current number of threads to fall to
> the desired level.
> 
Hmm. Really? I mean, is there really an effect when the semaphore 
waitqueue is active?
AFAICS down() on an semaphore with active waitqueue will always set the 
counter to -1; and as we don't have any thread corresponding to the 
down() we just issued the semaphore will continue starting threads once 
an up() is executed.

Probably not making myself clear here ...
The problem (from my point of view) with semaphores is that we don't 
have an direct counter of the number of processes waiting on that 
semaphore to become free. We do have a counter for the number of 
processes which are allowed to use the semaphore concurrently (namely 
->count), but the number of waiting processes must be gathered 
indirectly by the number of entries in the waitqueue.
Given enough processes in the waitqueue, the number of currently running 
processes effectively determines the number of processes to be started.
And as those processes are already running, I don't see an effective 
procedure how we could _reduce_ the number of processes to be started.

> But I don't really see a need to tune this on the fly - probably the worse
> problem occurs during bootup when the operator cannot perform tuning.
> 
Och aye, there is. If the calling parameters to call_usermodehelper are 
stored temporarily, you can allow for the call to return immediately 
when not enough resources are available. This way, it is even possible 
to stop khelper processes altogether (and effectively queueing all 
events) and re-enable the khelper processes at a later stage.

or you can throttle the number of khelper processes to some insanely 
small number (my test here runs with khelper_max=1), which lets you test 
your boot scripts for race conditions resp. hotplug awareness quite nicely.

> So a __setup parameter seems to be the best way of providing tunability. 
> Initialise the semaphore in usermodehelper_init().
> 
Which is what I've done.

THX Andrew, your feedback was _very_ welcome. Will wrap up a new patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&opÌk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Andrew Morton]

Hannes Reinecke <hare@suse.de> wrote:
>
>  The problem (from my point of view) with semaphores is that we don't 
>  have an direct counter of the number of processes waiting on that 
>  semaphore to become free. We do have a counter for the number of 
>  processes which are allowed to use the semaphore concurrently (namely 
>  ->count), but the number of waiting processes must be gathered 
>  indirectly by the number of entries in the waitqueue.

Well one could add the number of sleepers to the semaphore, or write some
function which counts the number of entries on the waitqueue or something. 
But the result of this query would be out-of-date as soon as the caller
obtained it and massaging all architectures would be needed.

I don't see why the "number of waiters" needs to be known for this problem.

>  Given enough processes in the waitqueue, the number of currently running 
>  processes effectively determines the number of processes to be started.
>  And as those processes are already running, I don't see an effective 
>  procedure how we could _reduce_ the number of processes to be started.

By reducing the number of processes which can concurrently take the
semaphore?  Confused.



-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Hannes Reinecke]

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

Andrew Morton wrote:
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> Given enough processes in the waitqueue, the number of currently running 
>> processes effectively determines the number of processes to be started.
>> And as those processes are already running, I don't see an effective 
>> procedure how we could _reduce_ the number of processes to be started.
> 
> 
> By reducing the number of processes which can concurrently take the
> semaphore?  Confused.
> 
Well, case in point: let's say we have khelper_max = 10, ten processes 
currently running, 15 processes in the waitqueue.
How can I reduce the number of concurrently running processes?
(Obviously not for the currently running processes, but for those in the 
waitqueue).
If I were to use down() as per your suggestion, I would have to use 
another helper thread as down() will block. Otherwise I will block the 
calling function (e.g. sysctl).
This just leads to added complexity; can't say I like it.

But then, your call. If you say 'use semaphores' I will do it.

Patch (for the semaphore version) is attached.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

[-- Attachment #2: khelper_restrict_maxnum_semaphore.patch --]
[-- Type: text/x-patch, Size: 14209 bytes --]

diff -u --recursive linux-2.6.8-rc1/include/linux/sysctl.h linux-2.6.8-rc1.hotplug/include/linux/sysctl.h
--- linux-2.6.8-rc1/include/linux/sysctl.h	2004-07-11 19:34:39.000000000 +0200
+++ linux-2.6.8-rc1.hotplug/include/linux/sysctl.h	2004-07-21 10:22:15.000000000 +0200
@@ -133,6 +133,7 @@
 	KERN_NGROUPS_MAX=63,	/* int: NGROUPS_MAX */
 	KERN_SPARC_SCONS_PWROFF=64, /* int: serial console power-off halt */
 	KERN_HZ_TIMER=65,	/* int: hz timer on or off */
+	KERN_KHELPER_MAX=66,    /* int: max # of concurrent khelper threads */
 };
 
 
diff -u --recursive linux-2.6.8-rc1/init/main.c linux-2.6.8-rc1.hotplug/init/main.c
--- linux-2.6.8-rc1/init/main.c	2004-07-11 19:33:56.000000000 +0200
+++ linux-2.6.8-rc1.hotplug/init/main.c	2004-07-23 09:43:35.000000000 +0200
@@ -93,6 +93,7 @@
 extern void populate_rootfs(void);
 extern void driver_init(void);
 extern void prepare_namespace(void);
+extern void usermodehelper_init(void);
 
 #ifdef CONFIG_TC
 extern void tc_init(void);
@@ -598,7 +599,9 @@
  */
 static void __init do_basic_setup(void)
 {
-	driver_init();
+	/* drivers will send hotplug events */
+	init_workqueues();
+	usermodehelper_init();
 
 #ifdef CONFIG_SYSCTL
 	sysctl_init();
@@ -607,7 +610,8 @@
 	/* Networking initialization needs a process context */ 
 	sock_init();
 
-	init_workqueues();
+	driver_init();
+
 	do_initcalls();
 }
 
diff -u --recursive linux-2.6.8-rc1/kernel/kmod.c linux-2.6.8-rc1.hotplug/kernel/kmod.c
--- linux-2.6.8-rc1/kernel/kmod.c	2004-07-11 19:34:19.000000000 +0200
+++ linux-2.6.8-rc1.hotplug/kernel/kmod.c	2004-07-27 10:42:49.000000000 +0200
@@ -17,6 +17,9 @@
 
 	call_usermodehelper wait flag, and remove exec_usermodehelper.
 	Rusty Russell <rusty@rustcorp.com.au>  Jan 2003
+
+	resource management for call_usermodehelper
+	Hannes Reinecke <hare@suse.de> Jul 2004
 */
 #define __KERNEL_SYSCALLS__
 
@@ -41,6 +44,10 @@
 extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
+int khelper_max = 50;
+static struct semaphore khelper_sem = __SEMAPHORE_INITIALIZER(khelper_sem, 50);
+
+#define DEBUG_KHELPER
 
 #ifdef CONFIG_KMOD
 
@@ -67,16 +74,12 @@
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
-	unsigned int max_modprobes;
 	int ret;
 	char *argv[] = { modprobe_path, "-q", "--", module_name, NULL };
 	static char *envp[] = { "HOME=/",
 				"TERM=linux",
 				"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
 				NULL };
-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
-	static int kmod_loop_msg;
 
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
@@ -95,21 +98,11 @@
 	 *
 	 * "trace the ppid" is simple, but will fail if someone's
 	 * parent exits.  I think this is as good as it gets. --RR
+	 *
+	 * Resource checking is now implemented in 
+	 * call_usermodehelper --hare
 	 */
-	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
-	atomic_inc(&kmod_concurrent);
-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg++ < 5)
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-		atomic_dec(&kmod_concurrent);
-		return -ENOMEM;
-	}
-
 	ret = call_usermodehelper(modprobe_path, argv, envp, 1);
-	atomic_dec(&kmod_concurrent);
 	return ret;
 }
 EXPORT_SYMBOL(request_module);
@@ -175,13 +168,166 @@
 	do_exit(0);
 }
 
+/* 
+ * Worker to adapt the number of khelper processes.
+ * down() might block, so we need a separate thread ...
+ */
+void khelper_modify_number(void *data)
+{
+	struct subprocess_info *sub_info = data;
+	int i, diff = sub_info->wait;
+	
+	if (khelper_max > 0) {
+		printk(KERN_INFO "khelper: max %d concurrent processes\n",
+		       khelper_max);
+	} else {
+		printk(KERN_INFO "khelper: delaying events\n");
+	}
+	/* Notify calling process */
+	sub_info->retval = 0;
+	complete(sub_info->complete);
+
+	/* Do the actual work and wait if neccessary */
+	if (diff < 0) {
+		for (i = 0; i > diff; i--)
+			down(&khelper_sem);
+	} else {
+		for (i = 0; i < diff; i++)
+			up(&khelper_sem);
+	}
+}
+
+/* simple wrapper to wake any sleeping processes */
+void khelper_notify(int diff)
+{
+	pid_t pid;
+	DECLARE_COMPLETION(done);
+	struct subprocess_info sub_info = {
+		.complete	= &done,
+		.path		= path,
+		.argv		= argv,
+		.envp		= envp,
+		.wait		= diff,
+		.retval		= 0,
+	};
+
+	pid = kernel_thread(khelper_modify_number, sub_info,
+			    CLONE_FS | CLONE_FILES | SIGCHLD);
+
+	if (pid >= 0)
+		wait_for_completion(&done);
+}
+
+/* 
+ * The process args are only valid until call_usermodehelper
+ * returns, so we have to copy them to somewhere safe.
+ */
+void khelper_copy_info(struct subprocess_info *orig, 
+		       struct subprocess_info *new)
+{
+	int i, l;
+	char *p;
+
+	new->path = kmalloc(4096, GFP_KERNEL);
+	if (!new->path)
+	    return;
+	memset(new->path, 0, 4096);
+	p = new->path;
+	strcpy(p, orig->path);
+	p += strlen(p);
+	p++;
+	new->argv = (char **)p;
+	i = 0;
+	l = 0;
+	while (orig->envp[i]) {
+		l += strlen(orig->envp[i]) + 1;
+		i++;
+	}
+	if ( i > 7 )
+		i = 7;
+	p += sizeof(char *) * (i + 1);
+
+	i = 0;
+	while (orig->argv[i] && i < 7) {
+		strcpy(p, orig->argv[i]);
+		new->argv[i] = p;
+		p += strlen(p);
+		*p++ = '\0';
+		i++;
+	}
+	new->argv[i] = NULL;
+
+	i = 0;
+	l = 0;
+	while (orig->envp[i]) {
+		l += strlen(orig->envp[i]) + 1;
+		i++;
+	}
+	if ( i > 31 )
+		i = 31;
+	new->envp = (char **)p;
+	p += sizeof(char *) * (i + 1);
+
+	i = 0;
+	while (orig->envp[i] && i < 31) {
+		strcpy(p, orig->envp[i]);
+		new->envp[i] = p;
+		p += strlen(p);
+		*p++ = '\0';
+		i++;
+	}
+	new->envp[i] = NULL;
+}
+
 /* Keventd can't block, but this (a child) can. */
 static int wait_for_helper(void *data)
 {
-	struct subprocess_info *sub_info = data;
+	struct subprocess_info *sub_info = data, stored_info;
 	pid_t pid;
 	struct k_sigaction sa;
+	int flags = SIGCHLD, retval;
+	char *ev_descr;
 
+	/* Copy process info, we might need it later on */
+	khelper_copy_info(sub_info, &stored_info);
+	if (!stored_info.path) {
+		sub_info->retval = -ENOMEM;
+		complete(sub_info->complete);
+		return 0;
+	}
+
+	stored_info.wait = sub_info->wait;
+#ifdef DEBUG_KHELPER
+	/* Debug info */
+	if (stored_info.wait) {
+		ev_descr = stored_info.argv[3];
+	} else {
+		ev_descr = stored_info.envp[3] + 7;
+	}
+#endif
+	if (down_trylock(&khelper_sem)) {
+		/* 
+		 * We have exceeded the maximum number of
+		 * concurrent kmod invocations. Delay this process
+		 * until enough resources are available again.
+		 */
+#ifdef DEBUG_KHELPER
+		printk(KERN_INFO "khelper: delay event %s (current %d, max %d)\n",
+		       ev_descr, atomic_read(&khelper_sem.count), khelper_max);
+#endif
+		/* Notify the caller */
+		stored_info.wait = -1;
+		sub_info->retval = -EAGAIN;
+		complete(sub_info->complete);
+		
+		/* Wait until the semaphore is available again */
+		down(&khelper_sem);
+	}
+	/* Do the real action */
+#ifdef DEBUG_KHELPER
+	printk(KERN_INFO "khelper: exec event %s (current %d, max %d)\n",
+	       ev_descr, atomic_read(&khelper_sem.count), khelper_max);
+#endif
 	/* Install a handler: if SIGCLD isn't handled sys_wait4 won't
 	 * populate the status, but will return -ECHILD. */
 	sa.sa.sa_handler = SIG_IGN;
@@ -190,23 +336,75 @@
 	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
 	allow_signal(SIGCHLD);
 
-	pid = kernel_thread(____call_usermodehelper, sub_info, SIGCHLD);
+	if (stored_info.wait < 1) {
+		/* CLONE_VFORK: wait until the usermode helper has execve'd
+		 * successfully We need the data structures to stay around
+		 * until that is done.  */
+		flags |= CLONE_VFORK;
+	}
+
+	pid = kernel_thread(____call_usermodehelper, &stored_info, flags);
 	if (pid < 0) {
-		sub_info->retval = pid;
-	} else {
+#ifdef DEBUG_KHELPER
+		printk(KERN_INFO "khelper: exec event %s failed (%d)\n",
+		       ev_descr, pid);
+#endif
+		kfree(stored_info.path);
+		if (stored_info.wait >= 0) {
+			sub_info->retval = pid;
+			complete(sub_info->complete);
+		}
+		/* Bail out */
+		if (atomic_read(&khelper_sem.count) < khelper_max)
+			up(&khelper_sem);
+		return 0;
+	}
+	/* 
+	 * usermodehelper started successfully
+	 * We always block for the child to exit as we want to
+	 * keep track of the number of currently running processes.
+	 */
+
+	if (stored_info.wait == 0) {
 		/*
-		 * Normally it is bogus to call wait4() from in-kernel because
-		 * wait4() wants to write the exit code to a userspace address.
-		 * But wait_for_helper() always runs as keventd, and put_user()
-		 * to a kernel address works OK for kernel threads, due to their
-		 * having an mm_segment_t which spans the entire address space.
-		 *
-		 * Thus the __user pointer cast is valid here.
+		 * For asynchronous events notify the caller
+		 * immediately, but wait for the event to finish.
 		 */
-		sys_wait4(pid, (int __user *) &sub_info->retval, 0, NULL);
+		complete(sub_info->complete);
 	}
 
-	complete(sub_info->complete);
+	/*
+	 * Normally it is bogus to call wait4() from in-kernel because
+	 * wait4() wants to write the exit code to a userspace address.
+	 * But wait_for_helper() always runs as keventd, and put_user()
+	 * to a kernel address works OK for kernel threads, due to their
+	 * having an mm_segment_t which spans the entire address space.
+	 *
+	 * Thus the __user pointer cast is valid here.
+	 */
+	sys_wait4(pid, (int __user *) &retval, 0, NULL);
+	
+	if (stored_info.wait > 0) {
+		/* 
+		 * For synchronous events we can return the exit
+		 * status of the child.
+		 */
+		sub_info->retval = retval;
+		complete(sub_info->complete);
+	}
+	
+	kfree(stored_info.path);
+
+	/* 
+	 * Check whether the maximum number of threads have
+	 * been reache and wake any sleeping kmod invocations
+	 * if there are enough resources available.
+	 * We have to test here as khelper_max might have been
+	 * changed per sysctl() whilst we have been sleeping.
+	 */
+	if (atomic_read(&khelper_sem.count) < khelper_max)
+		up(&khelper_sem);
+
 	return 0;
 }
 
@@ -216,21 +414,13 @@
 	struct subprocess_info *sub_info = data;
 	pid_t pid;
 
-	/* CLONE_VFORK: wait until the usermode helper has execve'd
-	 * successfully We need the data structures to stay around
-	 * until that is done.  */
-	if (sub_info->wait)
-		pid = kernel_thread(wait_for_helper, sub_info,
-				    CLONE_FS | CLONE_FILES | SIGCHLD);
-	else
-		pid = kernel_thread(____call_usermodehelper, sub_info,
-				    CLONE_VFORK | SIGCHLD);
+	pid = kernel_thread(wait_for_helper, sub_info,
+			    CLONE_FS | CLONE_FILES | SIGCHLD);
 
 	if (pid < 0) {
 		sub_info->retval = pid;
 		complete(sub_info->complete);
-	} else if (!sub_info->wait)
-		complete(sub_info->complete);
+	}
 }
 
 /**
@@ -272,10 +462,39 @@
 }
 EXPORT_SYMBOL(call_usermodehelper);
 
-static __init int usermodehelper_init(void)
+void __init usermodehelper_init(void)
 {
 	khelper_wq = create_singlethread_workqueue("khelper");
 	BUG_ON(!khelper_wq);
-	return 0;
+
+	/*
+	 * Limit the max number of concurrent processes
+	 * to something sane; 10 - max_threads/2 seems ok.
+	 */
+	if (khelper_max > max_threads/2)
+		khelper_max = max_threads/2;
+	if (khelper_max < 0)
+		khelper_max = 0;
+
+	if (khelper_max > 0) {
+		printk(KERN_INFO "khelper: max %d concurrent processes\n",
+		       khelper_max);
+	} else {
+		printk(KERN_INFO "khelper: delaying events\n");
+	}
+	atomic_set(&khelper_sem.count, khelper_max);
+}
+
+/*
+ * We want to restrict the maximum number of concurrent processes
+ * to max_threads / 2; however, at this time max_threads is not 
+ * yet initialised. So we will do the real check in usermodehelper_init().
+ */
+static int __init khelper_setup(char *s)
+{
+	get_option(&s, &khelper_max);
+
+	return 1;
 }
-core_initcall(usermodehelper_init);
+__setup("khelper_max=", khelper_setup);
+
diff -u --recursive linux-2.6.8-rc1/kernel/sysctl.c linux-2.6.8-rc1.hotplug/kernel/sysctl.c
--- linux-2.6.8-rc1/kernel/sysctl.c	2004-07-11 19:33:55.000000000 +0200
+++ linux-2.6.8-rc1.hotplug/kernel/sysctl.c	2004-07-27 10:12:59.000000000 +0200
@@ -77,6 +77,7 @@
 #ifdef CONFIG_HOTPLUG
 extern char hotplug_path[];
 #endif
+extern int khelper_max;
 #ifdef CONFIG_CHR_DEV_SG
 extern int sg_big_buff;
 #endif
@@ -124,6 +125,8 @@
 		       ctl_table *, void **);
 static int proc_doutsstring(ctl_table *table, int write, struct file *filp,
 		  void __user *buffer, size_t *lenp);
+static int proc_dointvec_khelper(ctl_table *table, int write, struct file *filp,
+				 void __user *buffer, size_t *lenp);
 
 static ctl_table root_table[];
 static struct ctl_table_header root_table_header =
@@ -387,6 +390,14 @@
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec,
 	},
+	{
+		.ctl_name	= KERN_KHELPER_MAX,
+		.procname	= "khelper_max",
+		.data		= &khelper_max,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec_khelper,
+	},
 #ifdef CONFIG_KMOD
 	{
 		.ctl_name	= KERN_MODPROBE,
@@ -1670,6 +1681,51 @@
 				do_proc_dointvec_minmax_conv, &param);
 }
 
+extern void khelper_notify(int);
+
+static int do_proc_dointvec_khelper_conv(int *negp, unsigned long *lvalp, 
+					 int *valp, 
+					 int write, void *data)
+{
+	struct do_proc_dointvec_minmax_conv_param *param = data;
+	if (write) {
+		int val = *negp ? -*lvalp : *lvalp;
+		int old = *valp;
+
+		if ((param->min && *param->min > val) ||
+		    (param->max && *param->max < val))
+		    return -EINVAL;
+		*valp = val;
+
+		/* Notify any sleeping processes */
+		khelper_notify(val - old);
+	} else {
+		int val = *valp;
+		if (val < 0) {
+			*negp = -1;
+			*lvalp = (unsigned long)-val;
+		} else {
+			*negp = 0;
+			*lvalp = (unsigned long)val;
+		}
+	}
+	return 0;
+}
+
+static int proc_dointvec_khelper(ctl_table *table, int write, struct file *filp,
+				 void __user *buffer, size_t *lenp)
+{
+	int khelper_max = max_threads / 2;
+
+	struct do_proc_dointvec_minmax_conv_param param = {
+		.min = (int *)&zero,
+		.max = (int *)&khelper_max,
+	};
+    
+	return do_proc_dointvec(table,write,filp,buffer,lenp,
+				do_proc_dointvec_khelper_conv, &param);
+}
+
 static int do_proc_doulongvec_minmax(ctl_table *table, int write,
 				     struct file *filp,
 				     void __user *buffer, size_t *lenp,

Re: [PATCH] Limit number of concurrent hotplug processes

[Andrew Morton]

Hannes Reinecke <hare@suse.de> wrote:
>
>  Patch (for the semaphore version) is attached.

err, what on earth is this patch trying to do?  It adds tons more
complexity then I expected to see.  Are the async (wait=0) semantics for
call_usermodehelper() preserved?

You cannot access semaphore.count here.  That's x86-specific.

Why is the code now doing

	if (stored_info.wait > 0) {
and
	if (stored_info.wait >= 0) {

?  `wait' is a boolean.  Or did its semantics get secretly changed somewhere?

Why is a new kernel thread needed to up and down a semaphore?

Sorry, but I've completely lost the plot on what you're trying to do here!




I'd have though that something like the below (untested, slightly hacky)
patch would suit.

diff -puN kernel/kmod.c~call_usermodehelper-ratelimiting kernel/kmod.c
--- 25/kernel/kmod.c~call_usermodehelper-ratelimiting	2004-07-27 01:10:02.621133160 -0700
+++ 25-akpm/kernel/kmod.c	2004-07-27 01:31:46.053981248 -0700
@@ -42,6 +42,12 @@ extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
 
+/*
+ * thread_count_sem is used to limit the number of concurrently-executing
+ * usermode helpers.
+ */
+static struct semaphore thread_count_sem;
+
 #ifdef CONFIG_KMOD
 
 /*
@@ -167,6 +173,8 @@ static int ____call_usermodehelper(void 
 	set_cpus_allowed(current, CPU_MASK_ALL);
 
 	retval = -EPERM;
+
+	current->flags |= PF_USERMODEHELPER;
 	if (current->fs->root)
 		retval = execve(sub_info->path, sub_info->argv,sub_info->envp);
 
@@ -175,6 +183,11 @@ static int ____call_usermodehelper(void 
 	do_exit(0);
 }
 
+void usermodehelper_exit(void)
+{
+	up(&thread_count_sem);
+}
+
 /* Keventd can't block, but this (a child) can. */
 static int wait_for_helper(void *data)
 {
@@ -193,6 +206,7 @@ static int wait_for_helper(void *data)
 	pid = kernel_thread(____call_usermodehelper, sub_info, SIGCHLD);
 	if (pid < 0) {
 		sub_info->retval = pid;
+		up(&thread_count_sem);
 	} else {
 		/*
 		 * Normally it is bogus to call wait4() from in-kernel because
@@ -228,6 +242,7 @@ static void __call_usermodehelper(void *
 
 	if (pid < 0) {
 		sub_info->retval = pid;
+		up(&thread_count_sem);
 		complete(sub_info->complete);
 	} else if (!sub_info->wait)
 		complete(sub_info->complete);
@@ -266,6 +281,7 @@ int call_usermodehelper(char *path, char
 	if (path[0] = '\0')
 		return 0;
 
+	down(&thread_count_sem);
 	queue_work(khelper_wq, &work);
 	wait_for_completion(&done);
 	return sub_info.retval;
@@ -274,6 +290,7 @@ EXPORT_SYMBOL(call_usermodehelper);
 
 void __init usermodehelper_init(void)
 {
+	sema_init(&thread_count_sem, 50);
 	khelper_wq = create_singlethread_workqueue("khelper");
 	BUG_ON(!khelper_wq);
 }
diff -puN include/linux/sched.h~call_usermodehelper-ratelimiting include/linux/sched.h
--- 25/include/linux/sched.h~call_usermodehelper-ratelimiting	2004-07-27 01:27:39.903401824 -0700
+++ 25-akpm/include/linux/sched.h	2004-07-27 01:28:06.429369264 -0700
@@ -578,6 +578,7 @@ do { if (atomic_dec_and_test(&(tsk)->usa
 #define PF_SWAPOFF	0x00080000	/* I am in swapoff */
 #define PF_LESS_THROTTLE 0x00100000	/* Throttle me less: I clean memory */
 #define PF_SYNCWRITE	0x00200000	/* I am doing a sync write */
+#define PF_USERMODEHELPER 0x00400000	/* kmod helper */
 
 #ifdef CONFIG_SMP
 #define SCHED_LOAD_SCALE	128UL	/* increase resolution of load */
diff -puN include/linux/kmod.h~call_usermodehelper-ratelimiting include/linux/kmod.h
--- 25/include/linux/kmod.h~call_usermodehelper-ratelimiting	2004-07-27 01:30:37.793358440 -0700
+++ 25-akpm/include/linux/kmod.h	2004-07-27 01:30:51.249312824 -0700
@@ -36,6 +36,8 @@ static inline int request_module(const c
 #define try_then_request_module(x, mod...) ((x) ?: (request_module(mod), (x)))
 extern int call_usermodehelper(char *path, char *argv[], char *envp[], int wait);
 
+void usermodehelper_exit(void);
+
 #ifdef CONFIG_HOTPLUG
 extern char hotplug_path [];
 #endif
diff -puN kernel/exit.c~call_usermodehelper-ratelimiting kernel/exit.c
--- 25/kernel/exit.c~call_usermodehelper-ratelimiting	2004-07-27 01:31:23.449417664 -0700
+++ 25-akpm/kernel/exit.c	2004-07-27 01:32:40.447712144 -0700
@@ -13,6 +13,7 @@
 #include <linux/completion.h>
 #include <linux/personality.h>
 #include <linux/tty.h>
+#include <linux/kmod.h>
 #include <linux/namespace.h>
 #include <linux/security.h>
 #include <linux/acct.h>
@@ -840,6 +841,9 @@ asmlinkage NORET_TYPE void do_exit(long 
 
 	tsk->exit_code = code;
 	exit_notify(tsk);
+	if (unlikely(tsk->flags & PF_USERMODEHELPER))
+		usermodehelper_exit();
+
 #ifdef CONFIG_NUMA
 	mpol_free(tsk->mempolicy);
 	tsk->mempolicy = NULL;
_



-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Hannes Reinecke]

Andrew Morton wrote:
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> Patch (for the semaphore version) is attached.
> 
> 
> err, what on earth is this patch trying to do?  It adds tons more
> complexity then I expected to see.  Are the async (wait=0) semantics for
> call_usermodehelper() preserved?
> 
Problem with your patch is that call_usermodehelper might block on 
down() regardless whether it is called async or sync.
So any write to sysfs which triggers a hotplug event might block until 
enough resources are available.

Most complexity is in fact due to the possibility to change khelper_max 
on the fly. If we disallow that everything else will be far cleaner.

> Why is the code now doing
> 
> 	if (stored_info.wait > 0) {
> and
> 	if (stored_info.wait >= 0) {
> 
> ?  `wait' is a boolean.  Or did its semantics get secretly changed somewhere?
> 
> Why is a new kernel thread needed to up and down a semaphore?
> 
As I said; down() might block. Unless we accept that the caller will 
only return after all down()s have been executed successfully we need 
something like that.

> Sorry, but I've completely lost the plot on what you're trying to do here!
> 
Sorry for this. I've probably pushed too hard for this.

I'll wrap up a patch which only allows for a static setting (via kernel 
command line parameters) and leave the on-the-fly setting for later :-).

> 
> I'd have though that something like the below (untested, slightly hacky)
> patch would suit.
> 
Indeed, but only if we accept that any call to call_usermodehelper might 
block if not enough resources are available.

THX for the patch, btw.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&opÌk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Andrew Morton]

Hannes Reinecke <hare@suse.de> wrote:
>
>  Problem with your patch is that call_usermodehelper might block on 
>  down() regardless whether it is called async or sync.
>  So any write to sysfs which triggers a hotplug event might block until 
>  enough resources are available.

This is exactly the behaviour we want.  If we don't block the caller then
the only option is to fail the call_usermodehelper() attempt, which would
be very bad indeed.

The main reason for calling call_usermodehelper(wait=0) is that the caller
holds locks which will prevent the helper application from terminating.  I
guess my proto-patch risks the same deadlock, because all the
currently-running helpers may be waiting on that lock.

Maybe this is why you were allocating a copy of the call_usermodehelper()
arguments?  I didn't try to reverse-engineer your patch to that extent -
I'd prefer to hear the design in your own words.  Am still waiting for
this, btw.

Allocating a whole bunch of buffers to hold copies of the
call_usermodehelper() args also uses resources, of course.  But it should
be acceptable.  We'd allocate the same amount of memory if we were sending
messages up a socket/pipe to userspace, which is what we should always have
done, instead of the direct-exec - it's caused tons of trouble.

You didn't answer half the questions in my previous email, btw.

Right now, I cannot think of any solution apart from:

- In the sync path, take the semaphore

- In the async path, take a copy of the args, then pass them over to some
  kernel thread which takes the args off a list one-at-a-time, takes the
  semaphore for each one, execs the helper and drops the semaphore on the
  exit path.


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] Limit number of concurrent hotplug processes

[Hannes Reinecke]

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

Andrew Morton wrote:
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> Problem with your patch is that call_usermodehelper might block on 
>> down() regardless whether it is called async or sync.
>> So any write to sysfs which triggers a hotplug event might block until 
>> enough resources are available.
> 
> 
> This is exactly the behaviour we want.  If we don't block the caller then
> the only option is to fail the call_usermodehelper() attempt, which would
> be very bad indeed.
> 
But only if call_usermodehelper is called with wait=1. For the async 
path we should not block.

> The main reason for calling call_usermodehelper(wait=0) is that the caller
> holds locks which will prevent the helper application from terminating.  I
> guess my proto-patch risks the same deadlock, because all the
> currently-running helpers may be waiting on that lock.
> 
And so it does. If an event is to be queued we need to copy the 
arguments and call 'complete' prior to calling down() to prevent locking 
of the calling process.

> Maybe this is why you were allocating a copy of the call_usermodehelper()
> arguments?  I didn't try to reverse-engineer your patch to that extent -
> I'd prefer to hear the design in your own words.  Am still waiting for
> this, btw.
> 
Ok. As already mentioned we have to copy the args if asynchronous events
are to be queued and wait for the semaphore to become available. Hence 
we will block and thus would need a similar mechanism as in 
wait_for_helper as otherwise either keventd itself or the calling thread 
would be blocked.
The only difference to the current wait_for_helper is that complete() 
should be called prior to calling down() to avoid blocking of the 
calling thread.
So part of my patch is just streamlining the calling sequence for 
usermodehelper to always call wait_for_helper and do all the main action 
in there.
In wait_for_helper I first check whether enough resources are available; 
if not the event is queued, wait=-1 and complete() is called to notify 
the calling function that this event is finished.
Here I mis-used the 'wait' entry so that -1 means 'event delayed'.
As we might have to queue the events I always copy the arguments to a 
local structure; thus we can have the same calling sequence for the 
entire function.

> Allocating a whole bunch of buffers to hold copies of the
> call_usermodehelper() args also uses resources, of course.  But it should
> be acceptable.  We'd allocate the same amount of memory if we were sending
> messages up a socket/pipe to userspace, which is what we should always have
> done, instead of the direct-exec - it's caused tons of trouble.
> 
Agreed. Would you be willing to accept patches for something like that?

> You didn't answer half the questions in my previous email, btw.
> 
Sorry. Hope this explains it a bit further.

> Right now, I cannot think of any solution apart from:
> 
> - In the sync path, take the semaphore
> 
> - In the async path, take a copy of the args, then pass them over to some
>   kernel thread which takes the args off a list one-at-a-time, takes the
>   semaphore for each one, execs the helper and drops the semaphore on the
>   exit path.
> 
Please have a look at the attached patch; it should be doing exactly this.

The patch to init/main.c is just for enabling all events with initramfs.
Without it events will be skipped as the workqueue for usermodehelper 
will be enabled only after any driver events have been generated.

Again, thanks very much for you help :-).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

[-- Attachment #2: khelper_restrict_maxnum_sem_take2.patch --]
[-- Type: text/x-patch, Size: 9074 bytes --]

diff -u --recursive linux-2.6.8-rc1/init/main.c linux-2.6.8-rc1.hotplug/init/main.c
--- linux-2.6.8-rc1/init/main.c	2004-07-11 19:33:56.000000000 +0200
+++ linux-2.6.8-rc1.hotplug/init/main.c	2004-07-23 09:43:35.000000000 +0200
@@ -93,6 +93,7 @@
 extern void populate_rootfs(void);
 extern void driver_init(void);
 extern void prepare_namespace(void);
+extern void usermodehelper_init(void);
 
 #ifdef CONFIG_TC
 extern void tc_init(void);
@@ -598,7 +599,9 @@
  */
 static void __init do_basic_setup(void)
 {
-	driver_init();
+	/* drivers will send hotplug events */
+	init_workqueues();
+	usermodehelper_init();
 
 #ifdef CONFIG_SYSCTL
 	sysctl_init();
@@ -607,7 +610,8 @@
 	/* Networking initialization needs a process context */ 
 	sock_init();
 
-	init_workqueues();
+	driver_init();
+
 	do_initcalls();
 }
 
diff -u --recursive linux-2.6.8-rc1/kernel/kmod.c linux-2.6.8-rc1.hotplug/kernel/kmod.c
--- linux-2.6.8-rc1/kernel/kmod.c	2004-07-11 19:34:19.000000000 +0200
+++ linux-2.6.8-rc1.hotplug/kernel/kmod.c	2004-07-27 14:55:26.000000000 +0200
@@ -17,6 +17,9 @@
 
 	call_usermodehelper wait flag, and remove exec_usermodehelper.
 	Rusty Russell <rusty@rustcorp.com.au>  Jan 2003
+
+	resource management for call_usermodehelper
+	Hannes Reinecke <hare@suse.de> Jul 2004
 */
 #define __KERNEL_SYSCALLS__
 
@@ -41,6 +44,8 @@
 extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
+int khelper_max = 50;
+static struct semaphore khelper_sem;
 
 #ifdef CONFIG_KMOD
 
@@ -67,16 +72,12 @@
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
-	unsigned int max_modprobes;
 	int ret;
 	char *argv[] = { modprobe_path, "-q", "--", module_name, NULL };
 	static char *envp[] = { "HOME=/",
 				"TERM=linux",
 				"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
 				NULL };
-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
-	static int kmod_loop_msg;
 
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
@@ -95,21 +96,11 @@
 	 *
 	 * "trace the ppid" is simple, but will fail if someone's
 	 * parent exits.  I think this is as good as it gets. --RR
+	 *
+	 * Resource checking is now implemented in 
+	 * call_usermodehelper --hare
 	 */
-	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
-	atomic_inc(&kmod_concurrent);
-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg++ < 5)
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-		atomic_dec(&kmod_concurrent);
-		return -ENOMEM;
-	}
-
 	ret = call_usermodehelper(modprobe_path, argv, envp, 1);
-	atomic_dec(&kmod_concurrent);
 	return ret;
 }
 EXPORT_SYMBOL(request_module);
@@ -167,6 +158,7 @@
 	set_cpus_allowed(current, CPU_MASK_ALL);
 
 	retval = -EPERM;
+
 	if (current->fs->root)
 		retval = execve(sub_info->path, sub_info->argv,sub_info->envp);
 
@@ -175,12 +167,99 @@
 	do_exit(0);
 }
 
+/* 
+ * The process args are only valid until call_usermodehelper
+ * returns, so we have to copy them to somewhere safe.
+ */
+void khelper_copy_info(struct subprocess_info *orig, 
+		       struct subprocess_info *new)
+{
+	int i, l;
+	char *p;
+
+	new->path = kmalloc(4096, GFP_KERNEL);
+	if (!new->path)
+	    return;
+	memset(new->path, 0, 4096);
+	p = new->path;
+	strcpy(p, orig->path);
+	p += strlen(p);
+	p++;
+	new->argv = (char **)p;
+	i = 0;
+	l = 0;
+	while (orig->envp[i]) {
+		l += strlen(orig->envp[i]) + 1;
+		i++;
+	}
+	if ( i > 7 )
+		i = 7;
+	p += sizeof(char *) * (i + 1);
+
+	i = 0;
+	while (orig->argv[i] && i < 7) {
+		strcpy(p, orig->argv[i]);
+		new->argv[i] = p;
+		p += strlen(p);
+		*p++ = '\0';
+		i++;
+	}
+	new->argv[i] = NULL;
+
+	i = 0;
+	l = 0;
+	while (orig->envp[i]) {
+		l += strlen(orig->envp[i]) + 1;
+		i++;
+	}
+	if ( i > 31 )
+		i = 31;
+	new->envp = (char **)p;
+	p += sizeof(char *) * (i + 1);
+
+	i = 0;
+	while (orig->envp[i] && i < 31) {
+		strcpy(p, orig->envp[i]);
+		new->envp[i] = p;
+		p += strlen(p);
+		*p++ = '\0';
+		i++;
+	}
+	new->envp[i] = NULL;
+}
+
 /* Keventd can't block, but this (a child) can. */
 static int wait_for_helper(void *data)
 {
-	struct subprocess_info *sub_info = data;
+	struct subprocess_info *sub_info = data, stored_info;
 	pid_t pid;
 	struct k_sigaction sa;
+	int flags = SIGCHLD, retval, wait = sub_info->wait;
+
+	/* Copy process info, we might need it later on */
+	khelper_copy_info(sub_info, &stored_info);
+	if (!stored_info.path) {
+		sub_info->retval = -ENOMEM;
+		complete(sub_info->complete);
+		return 0;
+	}
+
+	if (down_trylock(&khelper_sem)) {
+		/* 
+		 * We have exceeded the maximum number of
+		 * concurrent kmod invocations. Delay this process
+		 * until enough resources are available again.
+		 */
+		if (wait == 0) {
+			/* Queueing is for async events only */
+			wait = -1;
+			sub_info->retval = -EAGAIN;
+			complete(sub_info->complete);
+		}
+		/* Wait until the semaphore is available again */
+		down(&khelper_sem);
+	}
+	/* Do the real action */
 
 	/* Install a handler: if SIGCLD isn't handled sys_wait4 won't
 	 * populate the status, but will return -ECHILD. */
@@ -190,23 +269,62 @@
 	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
 	allow_signal(SIGCHLD);
 
-	pid = kernel_thread(____call_usermodehelper, sub_info, SIGCHLD);
+	if (wait < 1) {
+		/* CLONE_VFORK: wait until the usermode helper has execve'd
+		 * successfully We need the data structures to stay around
+		 * until that is done.  */
+		flags |= CLONE_VFORK;
+	}
+
+	pid = kernel_thread(____call_usermodehelper, &stored_info, flags);
 	if (pid < 0) {
-		sub_info->retval = pid;
-	} else {
+		if (wait >= 0) {
+			sub_info->retval = pid;
+			complete(sub_info->complete);
+		}
+		kfree(stored_info.path);
+		/* Bail out */
+		up(&khelper_sem);
+
+		return 0;
+	}
+	/* 
+	 * usermodehelper started successfully
+	 * We always block for the child to exit as we want to
+	 * keep track of the number of currently running processes.
+	 */
+
+	if (wait == 0) {
 		/*
-		 * Normally it is bogus to call wait4() from in-kernel because
-		 * wait4() wants to write the exit code to a userspace address.
-		 * But wait_for_helper() always runs as keventd, and put_user()
-		 * to a kernel address works OK for kernel threads, due to their
-		 * having an mm_segment_t which spans the entire address space.
-		 *
-		 * Thus the __user pointer cast is valid here.
+		 * For asynchronous events notify the caller
+		 * immediately, but wait for the event to finish.
 		 */
-		sys_wait4(pid, (int __user *) &sub_info->retval, 0, NULL);
+		complete(sub_info->complete);
+	}
+
+	/*
+	 * Normally it is bogus to call wait4() from in-kernel because
+	 * wait4() wants to write the exit code to a userspace address.
+	 * But wait_for_helper() always runs as keventd, and put_user()
+	 * to a kernel address works OK for kernel threads, due to their
+	 * having an mm_segment_t which spans the entire address space.
+	 *
+	 * Thus the __user pointer cast is valid here.
+	 */
+	sys_wait4(pid, (int __user *) &retval, 0, NULL);
+	
+	if (wait > 0) {
+		/* 
+		 * For synchronous events we can return the exit
+		 * status of the child.
+		 */
+		sub_info->retval = retval;
+		complete(sub_info->complete);
 	}
+	
+	kfree(stored_info.path);
+	up(&khelper_sem);
 
-	complete(sub_info->complete);
 	return 0;
 }
 
@@ -216,21 +334,13 @@
 	struct subprocess_info *sub_info = data;
 	pid_t pid;
 
-	/* CLONE_VFORK: wait until the usermode helper has execve'd
-	 * successfully We need the data structures to stay around
-	 * until that is done.  */
-	if (sub_info->wait)
-		pid = kernel_thread(wait_for_helper, sub_info,
-				    CLONE_FS | CLONE_FILES | SIGCHLD);
-	else
-		pid = kernel_thread(____call_usermodehelper, sub_info,
-				    CLONE_VFORK | SIGCHLD);
+	pid = kernel_thread(wait_for_helper, sub_info,
+			    CLONE_FS | CLONE_FILES | SIGCHLD);
 
 	if (pid < 0) {
 		sub_info->retval = pid;
 		complete(sub_info->complete);
-	} else if (!sub_info->wait)
-		complete(sub_info->complete);
+	}
 }
 
 /**
@@ -272,10 +382,35 @@
 }
 EXPORT_SYMBOL(call_usermodehelper);
 
-static __init int usermodehelper_init(void)
+void __init usermodehelper_init(void)
 {
 	khelper_wq = create_singlethread_workqueue("khelper");
 	BUG_ON(!khelper_wq);
-	return 0;
+
+	/*
+	 * Limit the max number of concurrent processes
+	 * to something sane; 10 - max_threads/2 seems ok.
+	 */
+	if (khelper_max > max_threads/2)
+		khelper_max = max_threads/2;
+	if (khelper_max < 10)
+		khelper_max = 10;
+
+	printk(KERN_INFO "khelper: max %d concurrent processes\n",
+	       khelper_max);
+	sema_init(&khelper_sem, khelper_max);
+}
+
+/*
+ * We want to restrict the maximum number of concurrent processes
+ * to max_threads / 2; however, at this time max_threads is not 
+ * yet initialised. So we will do the real check in usermodehelper_init().
+ */
+static int __init khelper_setup(char *s)
+{
+	get_option(&s, &khelper_max);
+
+	return 1;
 }
-core_initcall(usermodehelper_init);
+__setup("khelper_max=", khelper_setup);
+

Re: [PATCH] Limit number of concurrent hotplug processes

[Paul Jackson]

Andrew wrote:
> We'd allocate the same amount of memory if we were sending
> messages up a socket/pipe to userspace, which is what we should always have
> done, instead of the direct-exec - it's caused tons of trouble.

This touches on a question I have, off-topic to the discussion of
the patch proposed by Hannes.

Doesn't doing a direct-exec have one powerful advantage over sending
some message or kevent to userspace by socket/pipe/d-bus, in that
sending the message requires that there is some userspace code already
running that is competent to receive the message?

Doing the usermodehelper direct-exec _both_ provides the thread context
in which to execute, _and_ the code to be executed.  The alternative
seems to require long running threads, primed and ready to accept
particular events, cluttering up the system.

I will readily grant that this usermodehelper direct kernel exec thing
seems weird as all heck to me.  But I predict that in a couple of weeks
lkml will be seeing a patch from me (the next version of the 'cpuset'
patch I'm working with Simon and Sylvain of Bull) using it -- because
the alternative of a long running, rarely used, user thread just for one
obscure particular event that required user code sucked worse.

Am I missing something?  Are there _always_ better solutions than
usermodehelper's kernel direct-exec?

Or perhaps am I confusing what Andrew was referring to and the various
mechanisms available here in unfortunate ways?

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idG21&alloc_id\x10040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel