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: <20110906174939.GK7761@one.firstfloor.org>
Date:	Tue, 6 Sep 2011 19:49:39 +0200
From:	Andi Kleen <andi@...stfloor.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Andi Kleen <andi@...stfloor.org>,
	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 Tue, Sep 06, 2011 at 06:19:00PM +0200, Thomas Gleixner wrote:

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

Not convinced.

BTW can you at least merge the first patch for the notifiers. 
This fixes the "fixed hardware" which is currently broken too.

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

I don't understand. It only sets it if the new event is earlier.
So it can never be set back.

You seem to say the opposite?

> 
> Further the unprotected comparison on 32bit is completely bogus.

Ok.  Just need a ordered read like i_size_read().

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

Yes, I didn't audit that code. But probably it can be dropped
you're right.

-Andi
-- 
ak@...ux.intel.com -- Speaking for myself only.
--
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/