Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941086AbcKQObR (ORCPT ); Thu, 17 Nov 2016 09:31:17 -0500 Received: from mail-pg0-f66.google.com ([74.125.83.66]:36226 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936176AbcKQObK (ORCPT ); Thu, 17 Nov 2016 09:31:10 -0500 Date: Thu, 17 Nov 2016 22:31:00 +0800 From: Boqun Feng To: Lai Jiangshan 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 , edumazet@google.com, dvhart@linux.intel.com, =?iso-8859-1?Q?Fr=E9d=E9ric?= Weisbecker , oleg@redhat.com, bobby.prani@gmail.com, ldr709@gmail.com Subject: Re: [PATCH RFC tip/core/rcu] SRCU rewrite Message-ID: <20161117143012.GB5227@tardis.cn.ibm.com> References: <20161114183636.GA28589@linux.vnet.ibm.com> <20161115014445.GC12110@tardis.cn.ibm.com> <20161115143700.GZ4127@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="V88s5gaDVPzZ0KCq" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3344 Lines: 98 --V88s5gaDVPzZ0KCq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > >> > >> __srcu_read_lock() used to be called with preemption disabled. I guess > >> the reason was because we have two percpu variables to increase. So wi= th > >> only one percpu right, could we remove the preempt_{dis,en}able() in > >> srcu_read_lock() and use this_cpu_inc() here? > > > > Quite possibly... > > >=20 Hello, Lai ;-) > it will be nicer if it is removed. >=20 > 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. >=20 After reading the comment for a while, I actually got a question, maybe I miss something ;-) Why "at most NR_CPUS worth of readers using the old index haven't incremented the counters" could save us from overflow the counter? Please consider the following case in current implementation: {sp->completed =3D 0} so idx =3D 1 in srcu_advance_batches(...) one thread A is currently in __srcu_read_lock() and using idx =3D 1 and about to increase the percpu c[idx], and ULONG_MAX __srcu_read_lock()s have been called and returned with idx =3D 1, please note I think this is possible because I assume we may have some code like this: unsigned long i =3D 0; for (; i < ULONG_MAX; i++) srcu_read_lock(); // return the same idx 1; And none of the corresponding srcu_read_unlock() has been called; In this case, at the time thread A increases the percpu c[idx], that will result in an overflow, right? So even one reader using old idx will result in overflow. I think we won't be hit by overflow is not because we have few readers using old idx, it's because there are unlikely ULONG_MAX + 1 __srcu_read_lock() called for the same idx, right? And the reason of this is much complex: because we won't have a fair mount of threads in the system, because no thread will nest srcu many levels, because there won't be a lot readers using old idx. And this will still be true if we use new mechanism and shrink the preemption disabled section, right? Regards, Boqun > 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. >=20 > but we still need to think more if we want to remove the preempt_{dis,en}= able(). >=20 > Thanks > Lai --V88s5gaDVPzZ0KCq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYLb7+AAoJEEl56MO1B/q4gqYH/jvmagmUiTCZnP7nbC/DKlwg kqO6U5mJsjSDPa1VBOienchYcYFdNBGET5pvmi6W0FVnzNPJfBxMjDAUIURhHi2x Hl9gsHZSo5xJlvAPGiQ+iWcW5FWyyylow4raKlVdfCGlk30nukqtD9okzjPxXV4H m9hrkU2xg/Te2ZYFUu7/CYZrLlqlfheCPnnsyOaqTi8wVmEBhFG4HI134uEiGrfc yNZFdx53+rM5cfxEfuSpSq6PrAUvG41vDfwW1xv8kKhflMCj9TbLHNVRypVIVbPF 43gKS86fse8kLhz34GbzWPU7Lxfz/mdbaMQx45oOZMSyC7upljjV8a4rTUFjfO4= =zEWh -----END PGP SIGNATURE----- --V88s5gaDVPzZ0KCq--