Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, Jul 16, 2012 at 10:42:36AM -0000, Thomas Gleixner wrote:
> Provide a generic interface for setting up and tearing down percpu
> threads.
> 
> On registration the threads for already online cpus are created and
> started. On deregistration (modules) the threads are stoppped.
> 
> During hotplug operations the threads are created, started, parked and
> unparked. The datastructure for registration provides a pointer to
> percpu storage space and optional setup, cleanup, park, unpark
> functions. These functions are called when the thread state changes.
> 
> Each implementation has to provide a function which is queried and
> returns whether the thread should run and the thread function itself.
> 
> The core code handles all state transitions and avoids duplicated code
> in the call sites.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

This approach certainly results in much nicer code!

One issue noted below.

							Thanx, Paul

> +static int smpboot_thread_fn(void *data)
> +{
> +	struct smpboot_thread_data *td = data;
> +	struct smp_hotplug_thread *ht = td->ht;
> +
> +	while (1) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		preempt_disable();
> +		if (kthread_should_stop()) {
> +			set_current_state(TASK_RUNNING);
> +			preempt_enable();
> +			if (ht->cleanup)
> +				ht->cleanup(td->cpu, cpu_online(td->cpu));
> +			kfree(td);
> +			return 0;
> +		}
> +
> +		if (kthread_should_park()) {
> +			__set_current_state(TASK_RUNNING);
> +			preempt_enable();
> +			if (ht->park && td->status == HP_THREAD_ACTIVE) {
> +				BUG_ON(td->cpu != smp_processor_id());
> +				ht->park(td->cpu);
> +				td->status = HP_THREAD_PARKED;
> +			}
> +			kthread_parkme();
> +			/* We might have been woken for stop */
> +			continue;
> +		}
> +
> +		BUG_ON(td->cpu != smp_processor_id());
> +
> +		/* Check for state change setup */
> +		switch (td->status) {
> +		case HP_THREAD_NONE:
> +			preempt_enable();
> +			if (ht->setup)
> +				ht->setup(td->cpu);
> +			td->status = HP_THREAD_ACTIVE;
> +			preempt_disable();
> +			break;
> +		case HP_THREAD_PARKED:
> +			preempt_enable();
> +			if (ht->unpark)
> +				ht->unpark(td->cpu);
> +			td->status = HP_THREAD_ACTIVE;
> +			preempt_disable();
> +			break;
> +		}
> +
> +		if (!ht->thread_should_run(td->cpu)) {
> +			schedule_preempt_disabled();
> +		} else {
> +			set_current_state(TASK_RUNNING);
> +			preempt_enable();
> +			ht->thread_fn(td->cpu);
> +			preempt_disable();
> +		}

At this point, preemption is disabled, but the code at the beginning
of the loop will disable it again.  This results in "scheduling while
atomic" splats.  I did the following to clear them up:

+		if (!ht->thread_should_run(td->cpu)) {
+			preempt_enable();
+			schedule();
+		} else {
+			set_current_state(TASK_RUNNING);
+			preempt_enable();
+			ht->thread_fn(td->cpu);
+		}

I also placed the updated series on -rcu at branch rcu/smp/hotplug
(git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git),
based on tip/smp/hotplug, for Linaro testing purposes.

							Thanx, Paul