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

[Andi Kleen <andi@firstfloor.org>]

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@linux.intel.com -- Speaking for myself only.