On 11/07, Paul E. McKenney wrote:
>
> On Fri, Nov 02, 2012 at 07:06:29PM +0100, Oleg Nesterov wrote:
> > +void percpu_down_write(struct percpu_rw_semaphore *brw)
> > +{
> > + /* also blocks update_fast_ctr() which checks mutex_is_locked() */
> > + mutex_lock(&brw->writer_mutex);
> > +
> > + /*
> > + * 1. Ensures mutex_is_locked() is visible to any down_read/up_read
> > + * so that update_fast_ctr() can't succeed.
> > + *
> > + * 2. Ensures we see the result of every previous this_cpu_add() in
> > + * update_fast_ctr().
> > + *
> > + * 3. Ensures that if any reader has exited its critical section via
> > + * fast-path, it executes a full memory barrier before we return.
> > + */
> > + synchronize_sched();
> > +
> > + /* nobody can use fast_read_ctr, move its sum into slow_read_ctr */
> > + atomic_add(clear_fast_ctr(brw), &brw->slow_read_ctr);
> > +
> > + /* block the new readers completely */
> > + down_write(&brw->rw_sem);
> > +
> > + /* wait for all readers to complete their percpu_up_read() */
> > + wait_event(brw->write_waitq, !atomic_read(&brw->slow_read_ctr));
> > +}
> > +
> > +void percpu_up_write(struct percpu_rw_semaphore *brw)
> > +{
> > + /* allow the new readers, but only the slow-path */
> > + up_write(&brw->rw_sem);
> > +
> > + /* insert the barrier before the next fast-path in down_read */
> > + synchronize_sched();
>
> Ah, my added comments describing the memory-order properties of
> synchronize_sched() were incomplete. As you say in the comment above,
> a valid RCU implementation must ensure that each CPU executes a memory
> barrier between the time that synchronize_sched() starts executing and
> the time that this same CPU starts its first RCU read-side critical
> section that ends after synchronize_sched() finishes executing. (This
> is symmetric with the requirement discussed earlier.)
I think, yes. Let me repeat my example (changed a little bit). Suppose
that we have
int A = 0, B = 0, STOP = 0;
// can be called at any time, and many times
void func(void)
{
rcu_read_lock_sched();
if (!STOP) {
A++;
B++;
}
rcu_read_unlock_sched();
}
Then I believe the following code should be correct:
STOP = 1;
synchronize_sched();
BUG_ON(A != B);
We should see the result of the previous increments, and func() should
see STOP != 0 if it races with BUG_ON().
> And if a reader sees brw->writer_mutex as unlocked, then that reader's
> RCU read-side critical section must end after the above synchronize_sched()
> completes, which in turn means that there must have been a memory barrier
> on that reader's CPU after the synchronize_sched() started, so that the
> reader correctly sees the writer's updates.
Yes.
> But please let me know what you
> think of the added memory-order constraint.
I am going to (try to) do other changes on top of this patch, and I'll
certainly try to think more about this, thanks.
> Reviewed-by: Paul E. McKenney <paul...@linux.vnet.ibm.com>
Great! thanks a lot Paul.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
