From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mikulas Patocka Subject: Re: [PATCH] percpu-rwsem: use barrier in unlock path Date: Thu, 18 Oct 2012 12:05:19 -0400 (EDT) Message-ID: References: <507E48ED.8060809@cn.fujitsu.com> <20121017202806.GA7282@home.goodmis.org> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: Received: from mx1.redhat.com ([209.132.183.28]:61496 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753290Ab2JRQGi (ORCPT ); Thu, 18 Oct 2012 12:06:38 -0400 In-Reply-To: <20121017202806.GA7282@home.goodmis.org> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Steven Rostedt Cc: Lai Jiangshan , Linus Torvalds , Jens Axboe , linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, "Paul E. McKenney" , Peter Zijlstra , Thomas Gleixner , Eric Dumazet 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