From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932161AbbJZO4A (ORCPT ); Mon, 26 Oct 2015 10:56:00 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:56026 "EHLO e33.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932099AbbJZOz6 (ORCPT ); Mon, 26 Oct 2015 10:55:58 -0400 X-IBM-Helo: d03dlp01.boulder.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Mon, 26 Oct 2015 07:55:52 -0700 From: "Paul E. McKenney" To: Ingo Molnar Cc: linux-kernel@vger.kernel.org, jiangshanlai@gmail.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, dvhart@linux.intel.com, fweisbec@gmail.com, oleg@redhat.com, bobby.prani@gmail.com, Patrick Marlier Subject: Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference() Message-ID: <20151026145552.GG5105@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20151006161305.GA9799@linux.vnet.ibm.com> <1444148028-11551-1-git-send-email-paulmck@linux.vnet.ibm.com> <1444148028-11551-11-git-send-email-paulmck@linux.vnet.ibm.com> <20151026084506.GA28423@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20151026084506.GA28423@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15102614-0009-0000-0000-00000F3BA96B Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 26, 2015 at 09:45:06AM +0100, Ingo Molnar wrote: > > * Paul E. McKenney wrote: > > > From: Patrick Marlier > > > > The current list_entry_rcu() implementation copies the pointer to a stack > > variable, then invokes rcu_dereference_raw() on it. This results in an > > additional store-load pair. Now, most compilers will emit normal store > > and load instructions, which might seem to be of negligible overhead, > > but this results in a load-hit-store situation that can cause surprisingly > > long pipeline stalls, even on modern microprocessors. The problem is > > that it takes time for the store to get the store buffer updated, which > > can delay the subsequent load, which immediately follows. > > > > This commit therefore switches to the lockless_dereference() primitive, > > which does not expect the __rcu annotations (that are anyway not present > > in the list_head structure) and which, like rcu_dereference_raw(), > > does not check for an enclosing RCU read-side critical section. > > Most importantly, it does not copy the pointer, thus avoiding the > > load-hit-store overhead. > > > > Signed-off-by: Patrick Marlier > > [ paulmck: Switched to lockless_dereference() to suppress sparse warnings. ] > > Signed-off-by: Paul E. McKenney > > --- > > include/linux/rculist.h | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > > index 17c6b1f84a77..5ed540986019 100644 > > --- a/include/linux/rculist.h > > +++ b/include/linux/rculist.h > > @@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct list_head *list, > > * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock(). > > */ > > #define list_entry_rcu(ptr, type, member) \ > > -({ \ > > - typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \ > > - container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \ > > -}) > > + container_of(lockless_dereference(ptr), type, member) > > So this commit: > > 8db70b132dd5 ("rculist: Make list_entry_rcu() use lockless_dereference()") > > when merged with Linus's latest tree, triggers the following build failure on > allyesconfig/allmodconfig x86: > > triton:~/tip> make fs/fs-writeback.o > CHK include/config/kernel.release > CHK include/generated/uapi/linux/version.h > CHK include/generated/utsrelease.h > CHK include/generated/bounds.h > CHK include/generated/timeconst.h > CHK include/generated/asm-offsets.h > CALL scripts/checksyscalls.sh > CC fs/fs-writeback.o > In file included from fs/fs-writeback.c:16:0: > fs/fs-writeback.c: In function ‘bdi_split_work_to_wbs’: > include/linux/compiler.h:281:20: error: lvalue required as unary ‘&’ operand > __read_once_size(&(x), __u.__c, sizeof(x)); \ > ^ > include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’ > const typeof( ((type *)0)->member ) *__mptr = (ptr); \ > ^ > include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’ > #define READ_ONCE(x) __READ_ONCE(x, 1) > ^ > include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’ > typeof(p) _________p1 = READ_ONCE(p); \ > ^ > include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’ > container_of(lockless_dereference(ptr), type, member) > ^ > fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’ > struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list, > ^ > include/linux/compiler.h:283:28: error: lvalue required as unary ‘&’ operand > __read_once_size_nocheck(&(x), __u.__c, sizeof(x)); \ > ^ > include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’ > const typeof( ((type *)0)->member ) *__mptr = (ptr); \ > ^ > include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’ > #define READ_ONCE(x) __READ_ONCE(x, 1) > ^ > include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’ > typeof(p) _________p1 = READ_ONCE(p); \ > ^ > include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’ > container_of(lockless_dereference(ptr), type, member) > ^ > fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’ > struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list, > ^ > scripts/Makefile.build:258: recipe for target 'fs/fs-writeback.o' failed > make[1]: *** [fs/fs-writeback.o] Error 1 > Makefile:1526: recipe for target 'fs/fs-writeback.o' failed > make: *** [fs/fs-writeback.o] Error 2 > > It's this new usage in fs/fs-writeback.c: > > static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, > struct wb_writeback_work *base_work, > bool skip_if_busy) > { > struct bdi_writeback *last_wb = NULL; > struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list, I believe that the above should instead be: struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next, After all, RCU read-side list primitives need to fetch pointers in order to traverse those pointers in an RCU-safe manner. The patch below clears this up for me, does it also work for you? Thanx, Paul ------------------------------------------------------------------------ commit 9221c4e78cc3511cd15b1433617ae2548ad8f631 Author: Paul E. McKenney Date: Mon Oct 26 07:47:20 2015 -0700 fs: Fix argument to list_entry_rcu() The list_entry_rcu() macro requires that its first argument be the pointer to the list_head contained in the structure whose pointer is to be returned, but fetched in an RCU-safe manner. This commit therefore fixes the use in bdi_split_work_to_wbs() to follow this convention. Reported-by: Ingo Molnar Signed-off-by: Paul E. McKenney diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 29e4599f6fc1..126d4de8faad 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -779,7 +779,7 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, bool skip_if_busy) { struct bdi_writeback *last_wb = NULL; - struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list, + struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next, struct bdi_writeback, bdi_node); might_sleep();