Message-ID: <53353723.2040000@linux.vnet.ibm.com>
Date:	Fri, 28 Mar 2014 14:17:31 +0530
From:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
To:	Preeti U Murthy <preeti@...ux.vnet.ibm.com>
CC:	tglx@...utronix.de, mingo@...hat.com, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org, peterz@...radead.org, rjw@...ysocki.net,
	paulmck@...ux.vnet.ibm.com, davem@...emloft.net
Subject: Re: [PATCH] tick, broadcast: Prevent false alarm when force mask
 contains offline cpus

On 03/27/2014 03:44 PM, Preeti U Murthy wrote:
> On 03/27/2014 11:58 AM, Srivatsa S. Bhat wrote:
>>
>> Actually, my suggestion was to remove the dying CPU from the force_mask alone,
>> in the CPU_DYING notifier. The rest of the cleanup (removing it from the other
>> masks, moving the broadcast duty to someone else etc can still be done at
>> the CPU_DEAD stage). Also, note that the CPU which is set in force_mask is
>> definitely not the one doing the broadcast.
>>
>> Basically, my reasoning was this:
>>
>> If we look at how the 3 broadcast masks (oneshot, pending and force) are
>> set and cleared during idle entry/exit, we see this pattern:
>>
>> oneshot_mask: This is set at BROADCAST_ENTER and cleared at EXIT.
>> pending_mask: This is set at tick_handle_oneshot_broadcast and cleared at
>>               EXIT.
>> force_mask:   This is set at EXIT and cleared at the next call to
>>               tick_handle_oneshot_broadcast. (Also, if the CPU is set in this
>>               mask, the CPU doesn't enter deep idle states in subsequent
>>               idle durations, and keeps polling instead, until it gets the
>>               broadcast interrupt).
>>
>> What we can derive from this is that force_mask is the only mask that can
>> remain set across an idle ENTER/EXIT sequence. Both of the other 2 masks
>> can never remain set across a full idle ENTER/EXIT sequence. And a CPU going
>> offline certainly goes through EXIT if it had gone through ENTER, before
>> entering stop_machine().
>>
>> That means, force_mask is the only odd one out here, which can remain set
>> when entering stop_machine() for CPU offline. So that's the only mask that
>> needs to be cleared separately. The other 2 masks take care of themselves
>> automatically. So, we can have a CPU_DYING callback which just clears the
>> dying CPU from the force_mask (and does nothing more). That should work, no?
> 
> Yep I think this will work. Find the modified patch below:
> 
> Thanks.
> 
> Regards
> Preeti U Murthy
> 
> 
> tick,broadcast:Clear hotplugged cpu in broadcast masks during CPU_DYING notification
> 
> From: Preeti U Murthy <preeti@...ux.vnet.ibm.com>
> 
> Its possible that the tick_broadcast_force_mask contains cpus which are not
> in cpu_online_mask when a broadcast tick occurs. This could happen under the
> following circumstance assuming CPU1 is among the CPUs waiting for broadcast.
> 
> CPU0					CPU1
> 
> Run CPU_DOWN_PREPARE notifiers
> 
> Start stop_machine			Gets woken up by IPI to run
> 					stop_machine, sets itself in
> 					tick_broadcast_force_mask if the
> 					time of broadcast interrupt is around
> 					the same time as this IPI.
> 
> 					Start stop_machine
> 					  set_cpu_online(cpu1, false)
> End stop_machine			End stop_machine
> 
> Broadcast interrupt
>   Finds that cpu1 in
>   tick_broadcast_force_mask is offline
>   and triggers the WARN_ON in
>   tick_handle_oneshot_broadcast()
> 
> Clears all broadcast masks
> in CPU_DEAD stage.
> 
> While the hotplugged cpu clears its bit in the tick_broadcast_oneshot_mask
> and tick_broadcast_pending mask during BROADCAST_EXIT, it *sets* its bit
> in the tick_broadcast_force_mask if the broadcast interrupt is found to be
> around the same time as the present time. Today we clear all the broadcast
> masks and shutdown tick devices in the CPU_DEAD stage. But as shown above
> the broadcast interrupt could occur before this stage is reached and the
> WARN_ON() gets triggered when it is found that the tick_broadcast_force_mask
> contains an offline cpu.
> 
> This WARN_ON was added to capture scenarios where the broadcast mask, be it
> oneshot/pending/force_mask contain offline cpus whose tick devices have been
> removed. But here is a case where we trigger the WARN_ON() when the tick
> device of the hotplugged cpu is still around but we are delaying the clearing
> of the broadcast masks. This has not been a problem for
> tick_broadcastoneshot_mask and tick_broadcast_pending_mask since they get
> cleared on exit from broadcast.
>    But since the force_mask gets set at the same time on certain occasions
> it is necessary to move the clearing of masks to a stage during cpu hotplug
> before the hotplugged cpu clears itself in the online_mask.
>

That last sentence is not entirely accurate. During stop-machine in the CPU
offline path, the CPU removes itself from the cpu_online_mask at the very
beginning, in the __cpu_disable() call. Only after that the CPU_DYING notifiers
are invoked. But the advantage of clearing the CPU from the force_mask at
the CPU_DYING stage is that no other CPU is "noticing" this event, since
everybody is busy spinning in stop-machine. So, by the time stop-machine
completes and the CPU is officially offline, it would have "magically" cleared
itself from the force_mask as well, making things look very consistent for
the rest of the CPUs (i.e., an offline CPU will never remain set in the
force_mask).
 
> Hence move the clearing of broadcast masks to the CPU_DYING notification stage
> so that they remain consistent with the cpu_online_mask at the time of
> broadcast delivery at all times.
>

This last paragraph sums it up perfectly.
 
> Suggested-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@...ux.vnet.ibm.com>

You might want to alter the changelog a bit as mentioned above. Other than
that, everything looks fine to me. (But see one minor whitespace nitpick
below).

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>

> ---
>  kernel/time/clockevents.c    |    1 +
>  kernel/time/tick-broadcast.c |   20 +++++++++++++++-----
>  kernel/time/tick-internal.h  |    3 +++
>  3 files changed, 19 insertions(+), 5 deletions(-)
>
[...]
> @@ -912,11 +925,8 @@ void tick_shutdown_broadcast_oneshot(unsigned int *cpup)
>  	cpumask_clear_cpu(cpu, tick_broadcast_pending_mask);
>  	cpumask_clear_cpu(cpu, tick_broadcast_force_mask);
> 
> -	broadcast_move_bc(cpu);
> -
>  	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
>  }
> -

I guess you removed that newline by mistake. Please add it back, it improves
readability.

Regards,
Srivatsa S. Bhat

>  /*
>   * Check, whether the broadcast device is in one shot mode
>   */

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

Powered by blists - more mailing lists