From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751348AbdAODo5 (ORCPT ); Sat, 14 Jan 2017 22:44:57 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:48938 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751288AbdAODow (ORCPT ); Sat, 14 Jan 2017 22:44:52 -0500 Date: Sat, 14 Jan 2017 11:48:06 -0800 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, Lance Roy Subject: Re: [PATCH tip/core/rcu 1/3] srcu: More efficient reader counts. Reply-To: paulmck@linux.vnet.ibm.com References: <20170114091941.GA22961@linux.vnet.ibm.com> <1484385601-23379-1-git-send-email-paulmck@linux.vnet.ibm.com> <20170114093115.GA14970@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170114093115.GA14970@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17011503-0024-0000-0000-000015B35CF0 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006435; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000199; SDB=6.00807711; UDB=6.00393282; IPR=6.00585066; BA=6.00005056; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013923; XFM=3.00000011; UTC=2017-01-15 03:44:49 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17011503-0025-0000-0000-000047F46D87 Message-Id: <20170114194806.GV5238@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-15_03:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701150056 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jan 14, 2017 at 10:31:15AM +0100, Ingo Molnar wrote: > > Noticed a few minor nits: And thank you for the review and comments! > * Paul E. McKenney wrote: > > > From: Lance Roy > > > > SRCU uses two per-cpu counters: a nesting counter to count the number of > > active critical sections, and a sequence counter to ensure that the nesting > > counters don't change while they are being added together in > > srcu_readers_active_idx_check(). > > > > This patch instead uses per-cpu lock and unlock counters. Because the both > > counters only increase and srcu_readers_active_idx_check() reads the unlock > > counter before the lock counter, this achieves the same end without having > > to increment two different counters in srcu_read_lock(). This also saves a > > smp_mb() in srcu_readers_active_idx_check(). > > typo: > > s/Because the both counters > Because both counters Fixed! > > A possible problem with this patch is that it can only handle > > ULONG_MAX - NR_CPUS simultaneous readers, whereas the old version could > > handle up to ULONG_MAX. > > I don't think this is a problem! :-) Here is hoping! ;-) > > Suggested-by: Mathieu Desnoyers > > Signed-off-by: Lance Roy > > Signed-off-by: Paul E. McKenney > > Cc: Lai Jiangshan > > Cc: Peter Zijlstra > > --- > > include/linux/srcu.h | 4 +- > > kernel/rcu/rcutorture.c | 18 +++++++- > > kernel/rcu/srcu.c | 117 ++++++++++++++++++------------------------------ > > 3 files changed, 62 insertions(+), 77 deletions(-) > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > index dc8eb63c6568..0caea34d8c5f 100644 > > --- a/include/linux/srcu.h > > +++ b/include/linux/srcu.h > > @@ -34,8 +34,8 @@ > > #include > > > > struct srcu_struct_array { > > - unsigned long c[2]; > > - unsigned long seq[2]; > > + unsigned long lock_count[2]; > > + unsigned long unlock_count[2]; > > }; > > > > struct rcu_batch { > > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c > > index 87c51225ceec..6e4fd7680c70 100644 > > --- a/kernel/rcu/rcutorture.c > > +++ b/kernel/rcu/rcutorture.c > > @@ -564,10 +564,24 @@ static void srcu_torture_stats(void) > > pr_alert("%s%s per-CPU(idx=%d):", > > torture_type, TORTURE_FLAG, idx); > > for_each_possible_cpu(cpu) { > > + unsigned long l0, l1; > > + unsigned long u0, u1; > > long c0, c1; > > + struct srcu_struct_array* counts = > > + per_cpu_ptr(srcu_ctlp->per_cpu_ref, cpu); > > Please don't break the line to pacify checkpatch - if the line is too long then > maybe split out the loop body into a helper function - but keeping it a bit longer > than 80 cols is fine as well. Creating a helper function woujld leave me several characters over still, so I just created the long line. Another approach would be to split the definition and the initialization into two statements, but that would add a line. > > - c0 = (long)per_cpu_ptr(srcu_ctlp->per_cpu_ref, cpu)->c[!idx]; > > - c1 = (long)per_cpu_ptr(srcu_ctlp->per_cpu_ref, cpu)->c[idx]; > > + u0 = counts->unlock_count[!idx]; > > + u1 = counts->unlock_count[idx]; > > + > > + /* Make sure that a lock is always counted if the corresponding > > + unlock is counted. */ > > + smp_rmb(); > > That's not the standard kernel code comment style. That is embarrassing! Fixed. > > + > > + l0 = counts->lock_count[!idx]; > > + l1 = counts->lock_count[idx]; > > + > > + c0 = (long)(l0 - u0); > > + c1 = (long)(l1 - u1); > > These type casts look unnecessary to me. Indeed, given that we are assigning to a long rather than just computing. > > for_each_possible_cpu(cpu) { > > - t = READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->seq[idx]); > > + struct srcu_struct_array* cpu_counts = > > + per_cpu_ptr(sp->per_cpu_ref, cpu); > > + t = READ_ONCE(cpu_counts->lock_count[idx]); > > sum += t; > > > > for_each_possible_cpu(cpu) { > > - t = READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[idx]); > > + struct srcu_struct_array* cpu_counts = > > + per_cpu_ptr(sp->per_cpu_ref, cpu); > > + t = READ_ONCE(cpu_counts->unlock_count[idx]); > > sum += t; > > These linebreak look ugly as well. Some abbreviation of types and variables might > help: > > s/srcu_struct_array/srcu_array > s/cpu_counts/cpuc > > ? Why not? Fixed. ;-) > > + * If the locks are the same as the unlocks, then there must of have > > + * been no readers on this index at some time in between. This does not > > + * mean that there are no more readers, as one could have read the > > + * current index but have incremented the lock counter yet. > > > > + * Note that there can be at most NR_CPUS worth of readers using the old > > + * index that haven't incremented ->lock_count[] yet. Therefore, the > > + * sum of the ->lock_count[]s cannot increment enough times to overflow > > + * and end up equal the sum of the ->unlock_count[]s, as long as there > > + * are at most ULONG_MAX - NR_CPUS readers at a time. (Yes, this does > > + * mean that systems having more than a billion or so CPUs need to be > > + * 64-bit systems.) 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. > > typo: > > s/must of have been no readers/ > must have been no readers > > Also, maybe I'm misreading it, but shouldn't it be: > > s/as one could have read the current index but have incremented the lock counter yet. > /as one could have read the current index but not have incremented the lock counter yet. > > ? Agreed on both, fixed. > Also, the title: > > srcu: More efficient reader counts. > > should have a verb and no full stop, i.e. something like: > > srcu: Implement more efficient reader counts And this one as well. Thanx, Paul > Thanks, > > Ingo >