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
