linux-kernel - Re: [PATCH v3 2/2] CPU hotplug, stop-machine: Plug race-window that leads to "IPI-to-offline-CPU"

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]

Date:	Mon, 12 May 2014 16:57:57 -0400
From:	Tejun Heo <tj@...nel.org>
To:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc:	peterz@...radead.org, tglx@...utronix.de, mingo@...nel.org,
	rusty@...tcorp.com.au, akpm@...ux-foundation.org,
	fweisbec@...il.com, hch@...radead.org, mgorman@...e.de,
	riel@...hat.com, bp@...e.de, rostedt@...dmis.org,
	mgalbraith@...e.de, ego@...ux.vnet.ibm.com,
	paulmck@...ux.vnet.ibm.com, oleg@...hat.com, rjw@...ysocki.net,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] CPU hotplug, stop-machine: Plug race-window that
 leads to "IPI-to-offline-CPU"

Hello,

On Mon, May 12, 2014 at 02:07:04AM +0530, Srivatsa S. Bhat wrote:
> @@ -189,10 +191,27 @@ static int multi_cpu_stop(void *data)
>  	do {
>  		/* Chill out and ensure we re-read multi_stop_state. */
>  		cpu_relax();
> +
> +		/*
> +		 * In the case of CPU offline, we don't want the other CPUs to
> +		 * send IPIs to the active_cpu (the one going offline) after it
> +		 * has disabled interrupts in the _DISABLE_IRQ state (because,
> +		 * then it will notice the IPIs only after it goes offline). So
> +		 * we split this state into _INACTIVE and _ACTIVE, and thereby
> +		 * ensure that the active_cpu disables interrupts only after
> +		 * the other CPUs do the same thing.
> +		 */

It probably would be clearer to first describe what's going on and
then provide rationale for that.  IOW, state that inactive cpus
disable irqs first and then explain why that's done.  The above
paragraph looks somewhat out of place as is.

> +
>  		if (msdata->state != curstate) {
>  			curstate = msdata->state;
>  			switch (curstate) {
> -			case MULTI_STOP_DISABLE_IRQ:
> +			case MULTI_STOP_DISABLE_IRQ_INACTIVE:
> +				if (is_active)
> +					break;
> +
> +				/* Else, fall-through */
> +
> +			case MULTI_STOP_DISABLE_IRQ_ACTIVE:

Wouldn't it be cleaner to do the following?

	case MULTI_STOP_DISABLE_IRQ_INACTIVE:
		if (!is_active) {
			disable;
		}
		break;
	case MULTI_STOP_DISABLE_IRQ_ACTIVE:
		if (is_active) {
			disable;
		}
		break;

The duplicated amount is trivial and what's going on would be far
clearer.

Thanks.

-- 
tejun
--
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/