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

[Tejun Heo <tj@kernel.org>]

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