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 -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
