linux-kernel - Re: [PATCH 3/3] tick-broadcast: push down tick_broadcast

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]

Message-ID: <alpine.LFD.2.02.1109061652200.2723@ionos>
Date:	Tue, 6 Sep 2011 18:19:00 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Andi Kleen <andi@...stfloor.org>
cc:	LKML <linux-kernel@...r.kernel.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 3/3] tick-broadcast: push down tick_broadcast_lock

On Mon, 29 Aug 2011, Andi Kleen wrote:
> From: Andi Kleen <ak@...ux.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@...ux.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
--
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/