Re: Possible race condition during lock_count increment in srcu_read_lock

[Linu Cherian <linuc.decode@gmail.com>]

On Tue May 30, 2017 at 03:49:31PM +0200, Paolo Bonzini wrote:
> 
> 
> On 29/05/2017 22:14, Paul E. McKenney wrote:
> > On Mon, May 29, 2017 at 06:38:19PM +0200, Paolo Bonzini wrote:
> >> On 29/05/2017 17:50, Paul E. McKenney wrote:
> >>> You actually are not supposed to call __srcu_read_lock() and
> >>> __srcu_read_unlock() from irq context.  (Unless you know that you
> >>> interrupted some context that is guaranteed never to use the srcu_struct
> >>> in question or some such.)
> 
> I was going to improve the SRCU doc comments, and then started digging 
> into srcu.c history to find out why srcu_read_unlock uses this_cpu_inc.
> 
> It turns out that back in 2012 __srcu_read_lock did two accesses to 
> percpu variables, while __srcu_read_unlock did one (a decrement).  Then, 
> commit 5a41344a3d83 ("srcu: Simplify __srcu_read_unlock() via 
> this_cpu_dec()", 2012-11-29) changed __srcu_read_unlock to use 
> this_cpu_dec and remove the preempt_disable/preempt_enable pair.
> 
> Nowadays, __srcu_read_lock only does one access to percpu variables so 
> we could do the same as commit 5a41344a3d83 did:
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 167ad8831aaf..4c1d5f7e62c4 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -172,9 +172,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
>  {
>  	int retval;
>  
> -	preempt_disable();
>  	retval = __srcu_read_lock(sp);
> -	preempt_enable();
>  	rcu_lock_acquire(&(sp)->dep_map);
>  	return retval;
>  }
> diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
> index 584d8a983883..dea03614263f 100644
> --- a/kernel/rcu/srcu.c
> +++ b/kernel/rcu/srcu.c
> @@ -263,7 +263,7 @@ void cleanup_srcu_struct(struct srcu_struct *sp)
>  
>  /*
>   * Counts the new reader in the appropriate per-CPU element of the
> - * srcu_struct.  Must be called from process context.
> + * srcu_struct.
>   * Returns an index that must be passed to the matching srcu_read_unlock().
>   */
>  int __srcu_read_lock(struct srcu_struct *sp)
> @@ -271,7 +271,7 @@ int __srcu_read_lock(struct srcu_struct *sp)
>  	int idx;
>  
>  	idx = READ_ONCE(sp->completed) & 0x1;
> -	__this_cpu_inc(sp->per_cpu_ref->lock_count[idx]);
> +	this_cpu_inc(sp->per_cpu_ref->lock_count[idx]);
>  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
>  	return idx;
>  }
> @@ -281,7 +281,6 @@ int __srcu_read_lock(struct srcu_struct *sp)
>   * Removes the count for the old reader from the appropriate per-CPU
>   * element of the srcu_struct.  Note that this may well be a different
>   * CPU than that which was incremented by the corresponding srcu_read_lock().
> - * Must be called from process context.
>   */
>  void __srcu_read_unlock(struct srcu_struct *sp, int idx)
>  {
> 
> 
> As said earlier, this would slow down SRCU on machines where neither fast
> this_cpu_inc nor atomic_inc_relaxed are available, and local_irq_save/restore
> is slower than disabling preemption.  The main one is s390, which however is
> already paying the price in __srcu_read_unlock.
> 
> Any objections to the above for 4.12, with commit message explaining both the
> history of __srcu_read_unlock and what this fixes in KVM?
> 
> Thanks,
> 
> Paolo
> 

This works for me atleast on 4.11.2 kernel on Cavium arm64 platform.

Thanks.


-- 
Linu cherian