Noticed a few minor nits: * Paul E. McKenney <[email protected]> wrote:
> From: Lance Roy <[email protected]> > > 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 > > 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! :-) > > Suggested-by: Mathieu Desnoyers <[email protected]> > Signed-off-by: Lance Roy <[email protected]> > Signed-off-by: Paul E. McKenney <[email protected]> > Cc: Lai Jiangshan <[email protected]> > Cc: Peter Zijlstra <[email protected]> > --- > 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 <linux/workqueue.h> > > 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. > > - 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. > + > + 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. > 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 ? > + * 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. ? 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 Thanks, Ingo
