Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941809AbcKQSTw (ORCPT ); Thu, 17 Nov 2016 13:19:52 -0500 Received: from mail-it0-f67.google.com ([209.85.214.67]:33113 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933727AbcKQSTs (ORCPT ); Thu, 17 Nov 2016 13:19:48 -0500 MIME-Version: 1.0 In-Reply-To: <20161117144544.GC5227@tardis.cn.ibm.com> References: <20161114183636.GA28589@linux.vnet.ibm.com> <20161115014445.GC12110@tardis.cn.ibm.com> <20161115143700.GZ4127@linux.vnet.ibm.com> <20161117134957.GW3612@linux.vnet.ibm.com> <20161117143829.GA21600@linux.vnet.ibm.com> <20161117144544.GC5227@tardis.cn.ibm.com> From: Lai Jiangshan Date: Thu, 17 Nov 2016 23:55:07 +0800 Message-ID: Subject: Re: [PATCH RFC tip/core/rcu] SRCU rewrite To: Boqun Feng Cc: "Paul E. McKenney" , LKML , Ingo Molnar , dipankar@in.ibm.com, akpm@linux-foundation.org, Mathieu Desnoyers , Josh Triplett , Thomas Gleixner , Peter Zijlstra , Steven Rostedt , David Howells , Eric Dumazet , dvhart@linux.intel.com, =?UTF-8?B?RnLDqWTDqXJpYyBXZWlzYmVja2Vy?= , oleg@redhat.com, pranith kumar , ldr709@gmail.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2860 Lines: 63 On Thu, Nov 17, 2016 at 10:45 PM, Boqun Feng wrote: > On Thu, Nov 17, 2016 at 06:38:29AM -0800, Paul E. McKenney wrote: >> On Thu, Nov 17, 2016 at 05:49:57AM -0800, Paul E. McKenney wrote: >> > On Thu, Nov 17, 2016 at 08:18:51PM +0800, Lai Jiangshan wrote: >> > > On Tue, Nov 15, 2016 at 10:37 PM, Paul E. McKenney >> > > wrote: >> > > > On Tue, Nov 15, 2016 at 09:44:45AM +0800, Boqun Feng wrote: >> > > >> > > >> >> > > >> __srcu_read_lock() used to be called with preemption disabled. I guess >> > > >> the reason was because we have two percpu variables to increase. So with >> > > >> only one percpu right, could we remove the preempt_{dis,en}able() in >> > > >> srcu_read_lock() and use this_cpu_inc() here? >> > > > >> > > > Quite possibly... >> > > > >> > > >> > > it will be nicer if it is removed. >> > > >> > > The reason for the preemption-disabled was also because we >> > > have to disallow any preemption between the fetching of the idx >> > > and the increasement. so that we have at most NR_CPUS worth >> > > of readers using the old index that haven't incremented the counters. >> > > >> > > if we remove the preempt_{dis,en}able(). we must change the >> > > "NR_CPUS" in the comment into ULONG_MAX/4. (I assume >> > > one on-going reader needs at least need 4bytes at the stack). it is still safe. >> > > >> > > but we still need to think more if we want to remove the preempt_{dis,en}able(). >> > >> > Good points! Agreed, any change in the preemption needs careful thought >> > and needs to be a separate patch. >> >> And one area needing special thought is the call to __srcu_read_lock() >> and __srcu_read_unlock() in do_exit(). >> > > So before commit 49f5903b473c5, we don't have the read of ->completed in > preemption disable section? > > And following "git blame", I found commit 7a6b55e7108b3 ;-) Ouch, it shows 7a6b55e7108b3 at least has a bug in the comments about NR_CPUS. we should focus on the total number of all active readers instead the number of the readers using the old index that haven't incremented the counters. the later is smaller than the prior one which is smaller than the ULONG_MAX/4 or even smaller. so that we can simplify the comments. + * Note that the sum of the ->lock_count[]s cannot increment enough + * times to overflow and end up equal the sum of the ->unlock_count[]s, + * even too much readers using the old index that haven't incremented + * ->lock_count[] yet, as long as there are at most ULONG_MAX/4 + * readers at a time. Therefore, the only way that the return values of + * the two calls to srcu_readers_(un)lock_idx() can be equal is if there + * are no active readers using this index. > > Regards, > Boqun > >> Thanx, Paul >>