Messages in this thread |  | | | Date | Tue, 17 Feb 2009 00:19:46 +0100 | | From | Oleg Nesterov <> | | Subject | Re: Q: smp.c && barriers (Was: [PATCH 1/4] generic-smp: remove single ipi fallback for smp_call_function_many()) |
| |
On 02/16, Peter Zijlstra wrote: > > On Mon, 2009-02-16 at 23:02 +0100, Oleg Nesterov wrote: > > On 02/16, Peter Zijlstra wrote: > > > > > > On Mon, 2009-02-16 at 22:32 +0100, Oleg Nesterov wrote: > > > > > I was about to write a response, but found it to be a justification for > > > > > the read_barrier_depends() at the end of the loop. > > > > > > > > I forgot to mention I don't understand the read_barrier_depends() at the > > > > end of the loop as well ;) > > > > > > Suppose cpu0 adds to csd to cpu1: > > > > > > > > > cpu0: cpu1: > > > > > > add entry1 > > > mb(); > > > send ipi > > > run ipi handler > > > read_barrier_depends() > > > while (!list_empty()) [A] > > > do foo > > > > > > add entry2 > > > mb(); > > > [no ipi -- we still observe entry1] > > > > > > remove foo > > > read_barrier_depends() > > > while (!list_empty()) [B] > > > > Still can't understand. > > > > cpu1 (generic_smp_call_function_single_interrupt) does > > list_replace_init(q->lock), this lock is also taken by > > generic_exec_single(). > > > > Either cpu1 sees entry2 on list, or cpu0 sees list_empty() > > and sends ipi. > > cpu0: cpu1: > > spin_lock_irqsave(&dst->lock, flags); > ipi = list_empty(&dst->list); > list_add_tail(&data->list, &dst->list); > spin_unlock_irqrestore(&dst->lock, flags); > > ipi -----> > > while (!list_empty(&q->list)) { > unsigned int data_flags; > > spin_lock(&q->lock); > list_replace_init(&q->list, &list); > spin_unlock(&q->lock); > > > Strictly speaking the unlock() is semi-permeable, allowing the read of > q->list to enter the critical section, allowing us to observe an empty > list, never getting to q->lock on cpu1.
Hmm. If we take &q->lock, then we alread saw !list_empty() ?
And the question is, how can we miss list_empty() == F before spin_lock().
> > > The read_barrier_depends() matches the mb() on the other cpu, without > > > which the 'new' entry might not be observed. > > > > And that mb() looks unneeded too. Again, because > > generic_smp_call_function_single_interrupt() takes call_single_queue.lock > > before it uses "data".
to clarify, I meant it is not needed unless we are going to send the IPI. IOW, I think we can do
if (ipi) { /* Make the list addition visible before sending the ipi. */ wmb(); arch_send_call_function_single_ipi(cpu); } > > Even if I missed something (very possible), then I can't understand > > why we need rmb() only on alpha. > > Because only alpha is insane enough to do speculative reads? Dunno > really :-)
Perhaps...
It would be nice to have a comment which explains how can we miss the first addition without read_barrier_depends(). And why only on alpha.
And arch/alpha/kernel/smp.c:handle_ipi() does mb() itself...
Confused.
Oleg.
|  |