lkml.org 
[lkml]   [2014]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@linux.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@linux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@linux.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@linux.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
> */



\
 
 \ /
  Last update: 2014-03-28 10:01    [W:0.120 / U:5.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site