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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120721171259.GA6698@linux.vnet.ibm.com>
Date:	Sat, 21 Jul 2012 10:12:59 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Namhyung Kim <namhyung@...nel.org>
Subject: Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug
 threads

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@...utronix.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/