Limit number of concurrent hotplug processes

PATCH In review Hannes Reinecke 4 participants 13 messages Jul 20, 2004 – Jul 28, 2004

Subsystems (3)

Scheduler Proc sysctl Module support

lore

Share & tools

lore

Hannes Reinecke

author

2004-07-20 13:52 UTC #0 lore raw

PATCH 1/1 Limit number of concurrent hotplug processes

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

Signature · 4 lines

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

kernel/kmod.c renamed from kernel/kmod.c.orig +89 ~~-31~~

@@ -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);
+

Andrew Morton

2004-07-26 01:20 UTC #1 lore raw

Quoted 10 lines · Hannes Reinecke

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?

Quoted 1 line

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_id040&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Christian Borntraeger

2004-07-26 06:19 UTC #2 lore raw

Quoted 5 lines · Andrew Morton

Quoted 1 line · Hannes Reinecke
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

Hannes Reinecke

author

2004-07-26 10:59 UTC #3 lore raw

In reply to Andrew Morton

Quoted 8 lines · Andrew Morton

Quoted 6 lines · Hannes Reinecke
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.

Quoted 8 lines

Quoted 3 lines
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 ...

Quoted 10 lines

Quoted 2 lines
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);

Quoted 8 lines

{
	...
	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

Signature · 15 lines

Dr. Hannes Reinecke			[email protected]
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_id040&opÌk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Andrew Morton

2004-07-26 20:18 UTC #4 lore raw

Quoted 33 lines · Hannes Reinecke

Quoted 11 lines
Quoted 3 lines
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);
Quoted 8 lines
{
	...
	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.

Quoted 1 line

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().

Hannes Reinecke

author

2004-07-27 07:04 UTC #5 lore raw

Quoted 33 lines · Andrew Morton

Quoted 31 lines · Hannes Reinecke
Quoted 9 lines
Quoted 1 line
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);
Quoted 8 lines
{
	...
	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.

Quoted 8 lines

Quoted 1 line
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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Signature · 15 lines

Dr. Hannes Reinecke			[email protected]
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_id040&opÌk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Andrew Morton

2004-07-27 07:24 UTC #6 lore raw

Quoted 7 lines · Hannes Reinecke

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.

Quoted 4 lines

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.

Hannes Reinecke

author

2004-07-27 07:59 UTC #7 lore raw

Quoted 9 lines · Andrew Morton

Quoted 2 lines · Hannes Reinecke
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

Signature · 4 lines

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

include/linux/sysctl.h +1 -0

diff -u --recursive linux-2.6.8-rc1/include/linux/sysctl.h linux-2.6.8-rc1.hotplug/include/linux/sysctl.h
@@ -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 */
 };

init/main.c +6 -2

diff -u --recursive linux-2.6.8-rc1/init/main.c linux-2.6.8-rc1.hotplug/init/main.c
@@ -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();
 }

kernel/kmod.c +263 ~~-44~~

379 lines not on this page · raw message

kernel/sysctl.c +56 -0

80 lines not on this page · raw message

Andrew Morton

2004-07-27 08:34 UTC #8 lore raw

Quoted 2 lines · Hannes Reinecke

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.

kernel/kmod.c renamed from kernel/kmod.c~call_usermodehelper-ratelimiting +17 -0

diff -puN kernel/kmod.c~call_usermodehelper-ratelimiting kernel/kmod.c
@@ -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);
 }

include/linux/sched.h renamed from include/linux/sched.h~call_usermodehelper-ratelimiting +1 -0

diff -puN include/linux/sched.h~call_usermodehelper-ratelimiting include/linux/sched.h
@@ -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 */

include/linux/kmod.h renamed from include/linux/kmod.h~call_usermodehelper-ratelimiting +2 -0

diff -puN include/linux/kmod.h~call_usermodehelper-ratelimiting include/linux/kmod.h
@@ -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

kernel/exit.c renamed from kernel/exit.c~call_usermodehelper-ratelimiting +4 -0

diff -puN kernel/exit.c~call_usermodehelper-ratelimiting kernel/exit.c
@@ -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;

Hannes Reinecke

author

2004-07-27 09:05 UTC #9 lore raw

Quoted 9 lines · Andrew Morton

Quoted 1 line · Hannes Reinecke
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.

Quoted 10 lines

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.

Quoted 2 lines

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 :-).

Quoted 4 lines

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

Signature · 15 lines

Dr. Hannes Reinecke			[email protected]
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_id040&opÌk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Andrew Morton

2004-07-27 09:28 UTC #10 lore raw

Quoted 5 lines · Hannes Reinecke

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.

Hannes Reinecke

author

2004-07-27 12:22 UTC #11 lore raw

Quoted 10 lines · Andrew Morton

Quoted 2 lines · Hannes Reinecke
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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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?

Quoted 2 lines

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

Sorry. Hope this explains it a bit further.

Quoted 9 lines

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

Signature · 4 lines

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

init/main.c +6 -2

diff -u --recursive linux-2.6.8-rc1/init/main.c linux-2.6.8-rc1.hotplug/init/main.c
@@ -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();
 }

kernel/kmod.c +179 ~~-44~~

300 lines not on this page · raw message

Paul Jackson

2004-07-28 07:12 UTC #12 lore raw

In reply to Andrew Morton

Quoted 3 lines · Andrew

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?

Signature · 14 lines

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 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_id040&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel