On Sun, 19 Nov 2006, Oleg Nesterov wrote:
> > Put it this way: If the missing memory barrier in srcu_read_lock() after
> > the atomic_inc call isn't needed, then neither is the existing memory
> > barrier after the per-cpu counter gets incremented.
>
> I disagree. There is another reason for mb() after the per-cpu counter gets
> incremented. Without this barrier we can read the updated value of ->completed
> (incremented by synchronize_srcu()), but then read a stale data of the rcu
> protected memory.
You are right.
> > What you're ignoring is the synchronize_sched() call at the end of
> > synchronize_srcu(), which has been replaced with smp_mb(). The smp_mb()
> > needs to pair against a memory barrier on the read side, and that memory
> > barrier has to occur after srcu_read_lock() has incremented the counter
> > and before the read-side critical section begins. Otherwise code in the
> > critical section might leak out to before the counter is incremented.
>
> Still I am not sure you are right. It is ok (I think) if the code in the
> critical section leaks out to before the atomic_inc(). In fact this doesn't
> differ from the case when srcu_read_lock() happens before synchronize_srcu()
> starts. In that case synchronize_srcu() will wait until the critical section
> is closed via srcu_read_unlock(). Because of synchronize_sched()
> synchronize_srcu()
> can't miss the fact that the critical section is in progress, so it doesn't
> matter if it leaks _before_.
That's right. I was forgetting an important difference between the c[idx]
and the hardluckref paths: With c[idx] there has to be 2-way
communication (writer increments completed, then reader increments
c[idx]). With hardluckref there is only 1-way communication (reader
increments hardluckref). The synchronize_sched call takes care of the
reader->writer message; the memory barrier is needed for the
writer->reader message. Hence it isn't necessary after the atomic_inc.
But of course it _is_ needed for the fastpath to work. In fact, it might
not be good enough, depending on the architecture. Here's what the
fastpath ends up looking like (using c[idx] is essentially the same as
using hardluckref):
WRITER READER
------ ------
dataptr = &(new data) atomic_inc(&hardluckref)
mb mb
while (hardluckref > 0) ; access *dataptr
Notice the pattern: Each CPU does store-mb-load. It is known that on
some architectures each CPU can end up loading the old value (the value
from before the other CPU's store). This would mean the writer would see
hardluckref == 0 right away and the reader would see the old dataptr.
On architectures where the store-mb-load pattern works, the fastpath would
be safe. But on others it would not be.
Heh, Paul, this highlights the usefulness of our long discussion about
memory barrier semantics. :-)
Alan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
