Re: [PATCH v2 1/1] percpu_rw_semaphore: reimplement to not block the readers unnecessarily

[Oleg Nesterov <oleg@redhat.com>]

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 <paulmck@linux.vnet.ibm.com>

Great! thanks a lot Paul.

Oleg.