Re: [PATCH] percpu-rwsem: use barrier in unlock path

[Mikulas Patocka <mpatocka@redhat.com>]

On Wed, 17 Oct 2012, Steven Rostedt wrote:

> On Wed, Oct 17, 2012 at 11:07:21AM -0400, Mikulas Patocka wrote:
> > > 
> > > Even the previous patch is applied, percpu_down_read() still
> > > needs mb() to pair with it.
> > 
> > percpu_down_read uses rcu_read_lock which should guarantee that memory 
> > accesses don't escape in front of a rcu-protected section.
> 
> You do realize that rcu_read_lock() does nothing more that a barrier(),
> right?
> 
> Paul worked really hard to get rcu_read_locks() to not call HW barriers.
> 
> > 
> > If rcu_read_unlock has only an unlock barrier and not a full barrier, 
> > memory accesses could be moved in front of rcu_read_unlock and reordered 
> > with this_cpu_inc(*p->counters), but it doesn't matter because 
> > percpu_down_write does synchronize_rcu(), so it never sees these accesses 
> > halfway through.
> 
> Looking at the patch, you are correct. The read side doesn't need the
> memory barrier as the worse thing that will happen is that it sees the
> locked = false, and will just grab the mutex unnecessarily.

It wasn't correct.

CPU 1 is holding the write lock.

CPU 2 could get to percpu_down_read past rcu_read_lock and prefetch some 
data that are accessed after percpu_down_read.

CPU 1 goes into percpu_up_write(), calls a barrier, p->locked = false; and 
mutex_unlock(&p->mtx);

CPU 2 now sees p->locked == false, so it goes through percpu_down_read. It 
exists percpu_down_read and uses the invalid prefetched data.

It could be fixed by using synchronize_rcu(); in percpu_up_write, I sent a 
patch for that.

Mikulas