On Mon, Jun 12, 2017 at 02:17:59PM +0200, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
>
> The patch below does not apply to the 4.9-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@xxxxxxxxxxxxxxx>.
Indeed, this won't apply cleanly to 4.9 and earlier because of some
changes to the way SRCU works. And if you try to make the obvious
adjustments to make the patch apply, you will break SRCU.
So, question for Paolo... How important is it to push this back to
v4.9 and earlier? To make this happen, some non-trivial SRCU changes
would also need to be backported.
Thanx, Paul
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> >From 1123a6041654e8f889014659593bad4168e542c2 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Date: Wed, 31 May 2017 14:03:11 +0200
> Subject: [PATCH] srcu: Allow use of Classic SRCU from both process and
> interrupt context
>
> Linu Cherian reported a WARN in cleanup_srcu_struct() when shutting
> down a guest running iperf on a VFIO assigned device. This happens
> because irqfd_wakeup() calls srcu_read_lock(&kvm->irq_srcu) in interrupt
> context, while a worker thread does the same inside kvm_set_irq(). If the
> interrupt happens while the worker thread is executing __srcu_read_lock(),
> updates to the Classic SRCU ->lock_count[] field or the Tree SRCU
> ->srcu_lock_count[] field can be lost.
>
> The docs say you are not supposed to call srcu_read_lock() and
> srcu_read_unlock() from irq context, but KVM interrupt injection happens
> from (host) interrupt context and it would be nice if SRCU supported the
> use case. KVM is using SRCU here not really for the "sleepable" part,
> but rather due to its IPI-free fast detection of grace periods. It is
> therefore not desirable to switch back to RCU, which would effectively
> revert commit 719d93cd5f5c ("kvm/irqchip: Speed up KVM_SET_GSI_ROUTING",
> 2014-01-16).
>
> However, the docs are overly conservative. You can have an SRCU instance
> only has users in irq context, and you can mix process and irq context
> as long as process context users disable interrupts. In addition,
> __srcu_read_unlock() actually uses this_cpu_dec() on both Tree SRCU and
> Classic SRCU. For those two implementations, only srcu_read_lock()
> is unsafe.
>
> When Classic SRCU's __srcu_read_unlock() was changed to use this_cpu_dec(),
> in commit 5a41344a3d83 ("srcu: Simplify __srcu_read_unlock() via
> this_cpu_dec()", 2012-11-29), __srcu_read_lock() did two increments.
> Therefore it kept __this_cpu_inc(), with preempt_disable/enable in
> the caller. Tree SRCU however only does one increment, so on most
> architectures it is more efficient for __srcu_read_lock() to use
> this_cpu_inc(), and any performance differences appear to be down in
> the noise.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 719d93cd5f5c ("kvm/irqchip: Speed up KVM_SET_GSI_ROUTING")
> Reported-by: Linu Cherian <linuc.decode@xxxxxxxxx>
> Suggested-by: Linu Cherian <linuc.decode@xxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
>
> 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 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
>
> /*
> * 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 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
> * 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)
> {
>