Re: Q: smp.c && barriers (Was: [PATCH 1/4] generic-smp: remove single ipi fallback for smp_call_function_many())

[Nick Piggin <npiggin@suse.de>]

On Tue, Feb 17, 2009 at 11:27:33AM +0100, Peter Zijlstra wrote:
> On Tue, 2009-02-17 at 11:11 +0100, Nick Piggin wrote:
> 
> > But in that case, cpu0 should see list_empty and send another IPI,
> > because our load of list_empty has moved before the unlock of the
> > lock, so there can't be another item concurrently put on the list.
> 
> Suppose a first smp_call_function_single()
> 
> So cpu0 does:
> 
>  spin_lock(dst->lock);
>  ipi = list_empty(dst->list);
>  list_add_tail(data->list, dst->list);
>  spin_unlock(dst->lock);
> 
>  if (ipi) /* true */
>    send_single_ipi(cpu);
> 
> then cpu1 does:
> 
>  while (!list_empty(q->list))
> 
> and observes no entries, quits the ipi handler, and stuff is stuck.
> 
> cpu0 will observe a non-empty queue and will not raise another ipi, cpu1
> got the ipi, but observed no work and hence will not remove it.

Yes that's a valid case, but different from your one of the load
passing the spin_unlock inside the loop.

This one is interesting because we (the generic code) don't actually
quite know what we're ordering against. An IPI in some architecture
certainly could pass the cache coherency stream. But if that were the
case, then we have no generic primitives to handle it so I think it is
better to be enforced in arch code. Ie. cpu1 should always evaluate
to true in generic code with no additional barriers (and assuming that
a previous running IPI handler on cpu1 hasn't cleaned the list earlier).


> > But hmm, why even bother with all this complexity? Why not just
> > remove the outer loop completely? Do the lock and the list_replace_init
> > unconditionally. It would turn tricky lockless code into simple locked
> > code... we've already taken an interrupt anyway, so chances are pretty
> > high that we have work here to do, right?
> 
> Well, that's a practical suggestion, and I agree.
> 
> It was just fun arguing with Oleg ;-)

No arguments there ;)