Re: [PATCH tip/core/rcu 1/3] srcu: More efficient reader counts.

[Ingo Molnar]

Noticed a few minor nits:

* Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx> wrote:

> From: Lance Roy <ldr709@xxxxxxxxx>
> 
> 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 <mathieu.desnoyers@xxxxxxxxxxxx>
> Signed-off-by: Lance Roy <ldr709@xxxxxxxxx>
> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> ---
>  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