Re: [PATCH 3/3] tick-broadcast: push down tick_broadcast_lock

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 29 Aug 2011, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> For the oneshot case, only take the tick_broadcast_lock when the
> global device is actually changing. For the case when the new
> event is only setting the wakeup to a later time than it already
> is we don't need the lock.
> 
> This avoids lock contention for some special cases on systems
> that don't have an always running per cpu timer. It's not a full
> solution to the scalability problem there unfortunately, just
> the first step.

There is no full solution to that problem other than using sane
hardware.

> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  kernel/time/tick-broadcast.c |   18 ++++++++++++++----
>  1 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index 54a5977..7e748fb 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -485,23 +485,33 @@ void tick_broadcast_oneshot_control(unsigned long reason)
>  
>  	bc = tick_broadcast_device.evtdev;
>  
> -	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
>  	if (reason == CLOCK_EVT_NOTIFY_BROADCAST_ENTER) {
>  		if (!__get_cpu_var(state).need_oneshot) {
>  			__get_cpu_var(state).need_oneshot = 1;
>  			clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN);
> -			if (dev->next_event.tv64 < bc->next_event.tv64)
> +		       
> +			/* Only take the lock if the events gets set earlier */
> +			if (dev->next_event.tv64 < bc->next_event.tv64) {

That's racy and broken.

CPU0                                 CPU1

tick_handle_oneshot_broadcast()

 raw_spin_lock(&tick_broadcast_lock);
 bc->next_event = KTIME_MAX;
 for_each_online_cpu() {
	next_event = ...;
 }
 ....                                if (dev->next_event < bc->next_event) {
                                       raw_spin_lock(&tick_broadcast_lock);

 tick_broadcast_set_event(next_event, 0);
   bc->next_event = next_event;

 raw_spin_unlock(&tick_broadcast_lock);
                                       tick_broadcast_set_event(dev->next_event, 1);

So you unconditionally set the broadcast device to dev->next_event of
CPU1 even if the current pending event which was evaluated on CPU0 is
_BEFORE_ the CPU1 event. That can cause stalls and other hard to debug
horror. We've been there before.

Further the unprotected comparison on 32bit is completely bogus.

> +				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
>  				tick_broadcast_set_event(dev->next_event, 1);
> +				raw_spin_unlock_irqrestore(&tick_broadcast_lock, 
> +							   flags);		
> +			}
>  		}
>  	} else {
>  		if (__get_cpu_var(state).need_oneshot) {
>  			__get_cpu_var(state).need_oneshot = 0;
>  			clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT);
> -			if (dev->next_event.tv64 != KTIME_MAX)
> +
> +			/* Only take the lock if the event changes */
> +			if (dev->next_event.tv64 != KTIME_MAX) {
> +				raw_spin_lock_irqsave(&tick_broadcast_lock, flags);

Why would you take the global lock to program the cpu local device?
Just because it happened to be under that lock before?

>  				tick_program_event(dev->next_event, 1);
> +				raw_spin_unlock_irqrestore(&tick_broadcast_lock, 
> +							   flags);

Thanks,

	tglx