Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658181 > unrolled thread

[PATCH RFC tip/core/rcu 0/2] srcu: All SRCU readers from both process and irq

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC tip/core/rcu 0/2] srcu: All SRCU readers from both  process and irq "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 00:10 +0200
    [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 00:20 +0200
      Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 13:00 +0200
        Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 15:00 +0200
        Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Paolo Bonzini <pbonzini@redhat.com> - 2017-06-06 15:10 +0200
          Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Christian Borntraeger <borntraeger@de.ibm.com> - 2017-06-06 16:50 +0200
            Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-06-06 17:30 +0200
              Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Christian Borntraeger <borntraeger@de.ibm.com> - 2017-06-06 17:40 +0200
                Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 18:00 +0200
              Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 18:20 +0200
                Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 19:10 +0200
                Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-06-06 19:30 +0200
            Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 18:20 +0200
          Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 18:10 +0200
      Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 13:10 +0200
        Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Paolo Bonzini <pbonzini@redhat.com> - 2017-06-06 14:10 +0200
          Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 15:00 +0200
            Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 18:00 +0200
              Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 18:00 +0200
      Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 19:30 +0200
        Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 20:00 +0200
          Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context Peter Zijlstra <peterz@infradead.org> - 2017-06-06 20:10 +0200
            Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU  from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 20:30 +0200
    [PATCH RFC tip/core/rcu 2/2] srcu: Allow use of Classic SRCU from both process and interrupt context "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 00:20 +0200
    Re: [PATCH RFC tip/core/rcu 0/2] srcu: All SRCU readers from both  process and irq "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-06 19:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1659005 — Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU from both process and interrupt context

On Tue, Jun 06, 2017 at 07:23:42PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 05, 2017 at 03:09:50PM -0700, Paul E. McKenney wrote:
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 3ae8474557df..157654fa436a 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -357,7 +357,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)
> > @@ -365,7 +365,7 @@ int __srcu_read_lock(struct srcu_struct *sp)
> >  	int idx;
> >  
> >  	idx = READ_ONCE(sp->srcu_idx) & 0x1;
> > -	__this_cpu_inc(sp->sda->srcu_lock_count[idx]);
> > +	this_cpu_inc(sp->sda->srcu_lock_count[idx]);
> >  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
> >  	return idx;
> >  }
> 
> So again, the change is to make this an IRQ safe operation, however if
> we have this balance requirement, the IRQ will not visibly change the
> value and load-store should be good again, no?
> 
> Or am I missing some other detail with this implementation?

Unlike Tiny SRCU, Classic and Tree SRCU increment one counter
(->srcu_lock_count[]) and decrement another (->srcu_unlock_count[]).
So balanced srcu_read_lock() and srcu_read_unlock() within an irq
handler would increment both counters, with no decrements.  Therefore,
__srcu_read_lock()'s counter manipulation needs to be irq-safe.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1659017 — Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU from both process and interrupt context

On Tue, Jun 06, 2017 at 10:50:48AM -0700, Paul E. McKenney wrote:
> On Tue, Jun 06, 2017 at 07:23:42PM +0200, Peter Zijlstra wrote:
> > On Mon, Jun 05, 2017 at 03:09:50PM -0700, Paul E. McKenney wrote:
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 3ae8474557df..157654fa436a 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -357,7 +357,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)
> > > @@ -365,7 +365,7 @@ int __srcu_read_lock(struct srcu_struct *sp)
> > >  	int idx;
> > >  
> > >  	idx = READ_ONCE(sp->srcu_idx) & 0x1;
> > > -	__this_cpu_inc(sp->sda->srcu_lock_count[idx]);
> > > +	this_cpu_inc(sp->sda->srcu_lock_count[idx]);
> > >  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
> > >  	return idx;
> > >  }
> > 
> > So again, the change is to make this an IRQ safe operation, however if
> > we have this balance requirement, the IRQ will not visibly change the
> > value and load-store should be good again, no?
> > 
> > Or am I missing some other detail with this implementation?
> 
> Unlike Tiny SRCU, Classic and Tree SRCU increment one counter
> (->srcu_lock_count[]) and decrement another (->srcu_unlock_count[]).
> So balanced srcu_read_lock() and srcu_read_unlock() within an irq
> handler would increment both counters, with no decrements.  Therefore,
> __srcu_read_lock()'s counter manipulation needs to be irq-safe.

Oh, duh, so much for being able to read...

[toc] | [prev] | [next] | [standalone]

#1659043 — Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU from both process and interrupt context

On Tue, Jun 06, 2017 at 08:00:09PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 06, 2017 at 10:50:48AM -0700, Paul E. McKenney wrote:
> > On Tue, Jun 06, 2017 at 07:23:42PM +0200, Peter Zijlstra wrote:
> > > On Mon, Jun 05, 2017 at 03:09:50PM -0700, Paul E. McKenney wrote:
> > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > index 3ae8474557df..157654fa436a 100644
> > > > --- a/kernel/rcu/srcutree.c
> > > > +++ b/kernel/rcu/srcutree.c
> > > > @@ -357,7 +357,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)
> > > > @@ -365,7 +365,7 @@ int __srcu_read_lock(struct srcu_struct *sp)
> > > >  	int idx;
> > > >  
> > > >  	idx = READ_ONCE(sp->srcu_idx) & 0x1;
> > > > -	__this_cpu_inc(sp->sda->srcu_lock_count[idx]);
> > > > +	this_cpu_inc(sp->sda->srcu_lock_count[idx]);
> > > >  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
> > > >  	return idx;
> > > >  }
> > > 
> > > So again, the change is to make this an IRQ safe operation, however if
> > > we have this balance requirement, the IRQ will not visibly change the
> > > value and load-store should be good again, no?
> > > 
> > > Or am I missing some other detail with this implementation?
> > 
> > Unlike Tiny SRCU, Classic and Tree SRCU increment one counter
> > (->srcu_lock_count[]) and decrement another (->srcu_unlock_count[]).
> > So balanced srcu_read_lock() and srcu_read_unlock() within an irq
> > handler would increment both counters, with no decrements.  Therefore,
> > __srcu_read_lock()'s counter manipulation needs to be irq-safe.
> 
> Oh, duh, so much for being able to read...

I know that feeling!  Including the s/decrement/increment/ needed in my
erroneous paragraph above.  Classic and Tree SRCU increment both counters,
and they decrement nothing.  :-/

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1658189 — [PATCH RFC tip/core/rcu 2/2] srcu: Allow use of Classic SRCU from both process and interrupt context

From: Paolo Bonzini <pbonzini@redhat.com>

Linu Cherian reported a WARN in cleanup_srcu_struct when shutting
down a guest that has iperf running 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, lock_count can fall behind.

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 faster detection of grace periods, therefore it
is not possible to switch back to RCU, effectively reverting commit
719d93cd5f5c ("kvm/irqchip: Speed up KVM_SET_GSI_ROUTING", 2014-01-16).

However, the docs are painting a worse situation than it actually is.
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, so that only srcu_read_lock() is unsafe.

When srcuclassic'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.  Nowadays however it only does one increment, so on most
architectures it is more efficient for __srcu_read_lock to use
this_cpu_inc, too.

There would be a slowdown if 1) fast this_cpu_inc is not available and
cannot be implemented (this usually means that atomic_inc has implicit
memory barriers), and 2) local_irq_save/restore is slower than disabling
preemption.  The main architecture with these constraints is s390, which
however is already paying the price in __srcu_read_unlock and has not
complained.

Cc: stable@vger.kernel.org
Fixes: 719d93cd5f5c ("kvm/irqchip: Speed up KVM_SET_GSI_ROUTING")
Reported-by: Linu Cherian <linuc.decode@gmail.com>
Suggested-by: Linu Cherian <linuc.decode@gmail.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/srcu.h | 2 --
 kernel/rcu/srcu.c    | 5 ++---
 2 files changed, 2 insertions(+), 5 deletions(-)

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)
 {
-- 
2.5.2

[toc] | [prev] | [next] | [standalone]

#1658955

On Mon, Jun 05, 2017 at 03:09:19PM -0700, Paul E. McKenney wrote:
> This is a repost of a pair of patches from Paolo Bonzini to a wider
> audience.

And this is a repost of that repost, in response to review comments
for the first repost.  The main changes are:

1.	Drop the Tiny SRCU code changes.  As Peter Zijlstra pointed
	out, they are not needed.

2.	Update the commit logs to reflect the fact that any performance
	differences are expected to be down in the noise.

							Thanx, Paul

> Linu Cherian reported a WARN in cleanup_srcu_struct when shutting
> down a guest that has iperf running 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, lock_count can fall behind.  (KVM is using
> SRCU here not really for the "sleepable" part, but rather due to its
> faster detection of grace periods).  One way or another, this needs to
> be fixed in v4.12.
> 
> We discussed three ways of fixing this:
> 
> 1.	Have KVM protect process-level ->irq_srcu readers with
> 	local_irq_disable() or similar.  This works, and is the most
> 	confined change, but is a bit of an ugly usage restriction.
> 
> 2.	Make KVM convert ->irq_srcu uses to RCU-sched.	This works, but
> 	KVM needs fast grace periods, and synchronize_sched_expedited()
> 	interrupts CPUs, and is thus not particularly friendly to
> 	real-time workloads.
> 
> 3.	Make SRCU tolerate use of srcu_read_lock() and srcu_read_unlock()
> 	from both process context and irq handlers for the same
> 	srcu_struct.  It turns out that only a small change to SRCU is
> 	required, namely, changing __srcu_read_lock()'s __this_cpu_inc()
> 	to this_cpu_inc(), matching the existing __srcu_read_unlock()
> 	usage.	In addition, this change simplifies the use of SRCU.
> 	Of course, any RCU change after -rc1 is a bit scary.
> 	Nevertheless, the following two patches illustrate this approach.
> 
> Coward that I am, my personal preferred approach would be #1 during 4.12,
> possibly moving to #3 over time.  However, the KVM guys make a good case
> for just making a single small change right now and being done with it.
> Plus the overall effect of the one-step approach #3 is to make RCU
> smaller, even if only by five lines of code.
> 
> The reason for splitting this into two patches is to ease backporting.
> This means that the two commit logs are quite similar.
> 
> Thoughts?  In particular, are there better ways to fix this?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
>  include/linux/srcu.h     |    2 --
>  include/linux/srcutiny.h |    2 +-
>  kernel/rcu/rcutorture.c  |    4 ++--
>  kernel/rcu/srcu.c        |    5 ++---
>  kernel/rcu/srcutiny.c    |   21 ++++++++++-----------
>  kernel/rcu/srcutree.c    |    5 ++---
>  6 files changed, 17 insertions(+), 22 deletions(-)

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web