* [PATCH v2 0/2] sched: Add cond_resched_rcu_lock() helper @ 2013-04-30 2:52 Simon Horman 2013-04-30 2:52 ` [PATCH v2 1/2] " Simon Horman 2013-04-30 2:52 ` [PATCH v2 2/2] ipvs: Use cond_resched_rcu_lock() helper when dumping connections Simon Horman 0 siblings, 2 replies; 37+ messages in thread From: Simon Horman @ 2013-04-30 2:52 UTC (permalink / raw) To: Eric Dumazet, Julian Anastasov, Ingo Molnar, Peter Zijlstra, Paul E. McKenney Cc: lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma, Simon Horman Add a helper that for use in loops which read data protected by RCU and may have a large number of iterations. Such an example is dumping the list of connections known to IPVS: ip_vs_conn_array() and ip_vs_conn_seq_next(). This series also updates the two ip_vs functions mentioned above to use the helper. Changes since v1 noted in the changelog of each patch. Simon Horman (2): sched: Add cond_resched_rcu_lock() helper ipvs: Use cond_resched_rcu_lock() helper when dumping connections include/linux/sched.h | 11 +++++++++++ net/netfilter/ipvs/ip_vs_conn.c | 6 ++---- 2 files changed, 13 insertions(+), 4 deletions(-) -- 1.8.2.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-04-30 2:52 [PATCH v2 0/2] sched: Add cond_resched_rcu_lock() helper Simon Horman @ 2013-04-30 2:52 ` Simon Horman 2013-04-30 7:12 ` Julian Anastasov 2013-04-30 2:52 ` [PATCH v2 2/2] ipvs: Use cond_resched_rcu_lock() helper when dumping connections Simon Horman 1 sibling, 1 reply; 37+ messages in thread From: Simon Horman @ 2013-04-30 2:52 UTC (permalink / raw) To: Eric Dumazet, Julian Anastasov, Ingo Molnar, Peter Zijlstra, Paul E. McKenney Cc: lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma, Simon Horman This is intended for use in loops which read data protected by RCU and may have a large number of iterations. Such an example is dumping the list of connections known to IPVS: ip_vs_conn_array() and ip_vs_conn_seq_next(). The call to cond_resched() is guarded by #ifndef CONFIG_PREEMPT_RCU as in the case of CONFIG_PREEMPT_RCU _rcu_read_unlock() will check to see if the RCU core needs to be informed, so there is no need to invoke cond_resched() in that case. Thanks to Paul E. McKenney for explaining this. cond_resched_rcu_lock() suggested by Eric Dumazet. ifndef guard suggested by Paul E. McKenney and Julian Anastasov. Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com> Cc: Julian Anastasov <ja@ssi.bg> Acked-by: Ingo Molnar <mingo@kernel.org> Signed-off-by: Simon Horman <horms@verge.net.au> --- v2 * Add guard the call to cond_resched() --- include/linux/sched.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/include/linux/sched.h b/include/linux/sched.h index e692a02..66da71c 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -2787,3 +2787,14 @@ static inline unsigned long rlimit_max(unsigned int limit) } #endif + +static void inline cond_resched_rcu_lock(void) +{ + if (need_resched()) { + rcu_read_unlock(); +#ifndef CONFIG_PREEMPT_RCU + cond_resched(); +#endif + rcu_read_lock(); + } +} -- 1.8.2.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-04-30 2:52 ` [PATCH v2 1/2] " Simon Horman @ 2013-04-30 7:12 ` Julian Anastasov 2013-04-30 7:29 ` Simon Horman 0 siblings, 1 reply; 37+ messages in thread From: Julian Anastasov @ 2013-04-30 7:12 UTC (permalink / raw) To: Simon Horman Cc: Eric Dumazet, Ingo Molnar, Peter Zijlstra, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Tue, 30 Apr 2013, Simon Horman wrote: > This is intended for use in loops which read data protected by RCU and may > have a large number of iterations. Such an example is dumping the list of > connections known to IPVS: ip_vs_conn_array() and ip_vs_conn_seq_next(). > > The call to cond_resched() is guarded by #ifndef CONFIG_PREEMPT_RCU as in > the case of CONFIG_PREEMPT_RCU _rcu_read_unlock() will check to see if the > RCU core needs to be informed, so there is no need to invoke cond_resched() > in that case. Thanks to Paul E. McKenney for explaining this. > > cond_resched_rcu_lock() suggested by Eric Dumazet. > ifndef guard suggested by Paul E. McKenney and Julian Anastasov. > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > Cc: Julian Anastasov <ja@ssi.bg> > Acked-by: Ingo Molnar <mingo@kernel.org> > Signed-off-by: Simon Horman <horms@verge.net.au> > > --- > > v2 > * Add guard the call to cond_resched() > --- > include/linux/sched.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index e692a02..66da71c 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -2787,3 +2787,14 @@ static inline unsigned long rlimit_max(unsigned int limit) > } > > #endif > + > +static void inline cond_resched_rcu_lock(void) > +{ > + if (need_resched()) { Ops, it should be without above need_resched. > + rcu_read_unlock(); > +#ifndef CONFIG_PREEMPT_RCU > + cond_resched(); > +#endif > + rcu_read_lock(); > + } > +} > -- > 1.8.2.1 Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-04-30 7:12 ` Julian Anastasov @ 2013-04-30 7:29 ` Simon Horman 2013-04-30 7:52 ` Julian Anastasov 0 siblings, 1 reply; 37+ messages in thread From: Simon Horman @ 2013-04-30 7:29 UTC (permalink / raw) To: Julian Anastasov Cc: Eric Dumazet, Ingo Molnar, Peter Zijlstra, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Tue, Apr 30, 2013 at 10:12:38AM +0300, Julian Anastasov wrote: > > Hello, > > On Tue, 30 Apr 2013, Simon Horman wrote: > > > This is intended for use in loops which read data protected by RCU and may > > have a large number of iterations. Such an example is dumping the list of > > connections known to IPVS: ip_vs_conn_array() and ip_vs_conn_seq_next(). > > > > The call to cond_resched() is guarded by #ifndef CONFIG_PREEMPT_RCU as in > > the case of CONFIG_PREEMPT_RCU _rcu_read_unlock() will check to see if the > > RCU core needs to be informed, so there is no need to invoke cond_resched() > > in that case. Thanks to Paul E. McKenney for explaining this. > > > > cond_resched_rcu_lock() suggested by Eric Dumazet. > > ifndef guard suggested by Paul E. McKenney and Julian Anastasov. > > > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > Cc: Julian Anastasov <ja@ssi.bg> > > Acked-by: Ingo Molnar <mingo@kernel.org> > > Signed-off-by: Simon Horman <horms@verge.net.au> > > > > --- > > > > v2 > > * Add guard the call to cond_resched() > > --- > > include/linux/sched.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index e692a02..66da71c 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -2787,3 +2787,14 @@ static inline unsigned long rlimit_max(unsigned int limit) > > } > > > > #endif > > + > > +static void inline cond_resched_rcu_lock(void) > > +{ > > + if (need_resched()) { > > Ops, it should be without above need_resched. Thanks, to clarify, just this: static void inline cond_resched_rcu_lock(void) { rcu_read_unlock(); #ifndef CONFIG_PREEMPT_RCU cond_resched(); #endif rcu_read_lock(); } ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-04-30 7:29 ` Simon Horman @ 2013-04-30 7:52 ` Julian Anastasov 2013-05-01 9:10 ` Peter Zijlstra 0 siblings, 1 reply; 37+ messages in thread From: Julian Anastasov @ 2013-04-30 7:52 UTC (permalink / raw) To: Simon Horman Cc: Eric Dumazet, Ingo Molnar, Peter Zijlstra, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Tue, 30 Apr 2013, Simon Horman wrote: > > > +static void inline cond_resched_rcu_lock(void) > > > +{ > > > + if (need_resched()) { > > > > Ops, it should be without above need_resched. > > Thanks, to clarify, just this: > > static void inline cond_resched_rcu_lock(void) > { > rcu_read_unlock(); > #ifndef CONFIG_PREEMPT_RCU > cond_resched(); > #endif > rcu_read_lock(); > } Yes, thanks! -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-04-30 7:52 ` Julian Anastasov @ 2013-05-01 9:10 ` Peter Zijlstra 2013-05-01 12:46 ` Paul E. McKenney 2013-05-01 14:22 ` Julian Anastasov 0 siblings, 2 replies; 37+ messages in thread From: Peter Zijlstra @ 2013-05-01 9:10 UTC (permalink / raw) To: Julian Anastasov Cc: Simon Horman, Eric Dumazet, Ingo Molnar, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Tue, Apr 30, 2013 at 10:52:38AM +0300, Julian Anastasov wrote: > > Hello, > > On Tue, 30 Apr 2013, Simon Horman wrote: > > > > > +static void inline cond_resched_rcu_lock(void) > > > > +{ > > > > + if (need_resched()) { > > > > > > Ops, it should be without above need_resched. > > > > Thanks, to clarify, just this: > > > > static void inline cond_resched_rcu_lock(void) > > { > > rcu_read_unlock(); > > #ifndef CONFIG_PREEMPT_RCU > > cond_resched(); > > #endif > > rcu_read_lock(); > > } > > Yes, thanks! OK, now I'm confused.. PREEMPT_RCU would preempt in any case, so why bother dropping rcu_read_lock() at all? That is; the thing that makes sense to me is: static void inline cond_resched_rcu_lock(void) { #ifdef CONFIG_PREEMPT_RCU if (need_resched()) { rcu_read_unlock(); cond_resched(); rcu_read_lock(); } #endif /* CONFIG_PREEMPT_RCU */ } That would have an rcu_read_lock() break and voluntary preemption point for non-preemptible RCU and not bother with the stuff for preemptible RCU. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 9:10 ` Peter Zijlstra @ 2013-05-01 12:46 ` Paul E. McKenney 2013-05-01 14:32 ` Paul E. McKenney 2013-05-01 15:17 ` Peter Zijlstra 2013-05-01 14:22 ` Julian Anastasov 1 sibling, 2 replies; 37+ messages in thread From: Paul E. McKenney @ 2013-05-01 12:46 UTC (permalink / raw) To: Peter Zijlstra Cc: Julian Anastasov, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 11:10:12AM +0200, Peter Zijlstra wrote: > On Tue, Apr 30, 2013 at 10:52:38AM +0300, Julian Anastasov wrote: > > > > Hello, > > > > On Tue, 30 Apr 2013, Simon Horman wrote: > > > > > > > +static void inline cond_resched_rcu_lock(void) > > > > > +{ > > > > > + if (need_resched()) { > > > > > > > > Ops, it should be without above need_resched. > > > > > > Thanks, to clarify, just this: > > > > > > static void inline cond_resched_rcu_lock(void) > > > { > > > rcu_read_unlock(); > > > #ifndef CONFIG_PREEMPT_RCU > > > cond_resched(); > > > #endif > > > rcu_read_lock(); > > > } > > > > Yes, thanks! > > OK, now I'm confused.. PREEMPT_RCU would preempt in any case, so why bother > dropping rcu_read_lock() at all? Good point, I was assuming that the goal was to let grace periods end as well as to allow preemption. The momentary dropping out of the RCU read-side critical section allows the grace periods to end. > That is; the thing that makes sense to me is: > > static void inline cond_resched_rcu_lock(void) > { > #ifdef CONFIG_PREEMPT_RCU > if (need_resched()) { > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > } > #endif /* CONFIG_PREEMPT_RCU */ > } > > That would have an rcu_read_lock() break and voluntary preemption point for > non-preemptible RCU and not bother with the stuff for preemptible RCU. If the only goal is to allow preemption, and if long grace periods are not a concern, then this alternate approach would work fine as well. Of course, both approaches assume that the caller is in a place where having all RCU-protected data disappear is OK! Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 12:46 ` Paul E. McKenney @ 2013-05-01 14:32 ` Paul E. McKenney 2013-05-02 7:27 ` Peter Zijlstra 2013-05-01 15:17 ` Peter Zijlstra 1 sibling, 1 reply; 37+ messages in thread From: Paul E. McKenney @ 2013-05-01 14:32 UTC (permalink / raw) To: Peter Zijlstra Cc: Julian Anastasov, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 05:46:37AM -0700, Paul E. McKenney wrote: > On Wed, May 01, 2013 at 11:10:12AM +0200, Peter Zijlstra wrote: > > On Tue, Apr 30, 2013 at 10:52:38AM +0300, Julian Anastasov wrote: > > > > > > Hello, > > > > > > On Tue, 30 Apr 2013, Simon Horman wrote: > > > > > > > > > +static void inline cond_resched_rcu_lock(void) > > > > > > +{ > > > > > > + if (need_resched()) { > > > > > > > > > > Ops, it should be without above need_resched. > > > > > > > > Thanks, to clarify, just this: > > > > > > > > static void inline cond_resched_rcu_lock(void) > > > > { > > > > rcu_read_unlock(); > > > > #ifndef CONFIG_PREEMPT_RCU > > > > cond_resched(); > > > > #endif > > > > rcu_read_lock(); > > > > } > > > > > > Yes, thanks! > > > > OK, now I'm confused.. PREEMPT_RCU would preempt in any case, so why bother > > dropping rcu_read_lock() at all? > > Good point, I was assuming that the goal was to let grace periods end > as well as to allow preemption. The momentary dropping out of the > RCU read-side critical section allows the grace periods to end. > > > That is; the thing that makes sense to me is: > > > > static void inline cond_resched_rcu_lock(void) > > { > > #ifdef CONFIG_PREEMPT_RCU > > if (need_resched()) { > > rcu_read_unlock(); > > cond_resched(); > > rcu_read_lock(); > > } > > #endif /* CONFIG_PREEMPT_RCU */ > > } > > > > That would have an rcu_read_lock() break and voluntary preemption point for > > non-preemptible RCU and not bother with the stuff for preemptible RCU. > > If the only goal is to allow preemption, and if long grace periods are > not a concern, then this alternate approach would work fine as well. But now that I think about it, there is one big advantage to the unconditional exiting and reentering the RCU read-side critical section: It allows easy placement of unconditional lockdep debug code to catch the following type of bug: rcu_read_lock(); ... rcu_read_lock(); ... cond_resched_rcu_lock(); ... rcu_read_unlock(); ... rcu_read_unlock(); Here is how to detect this: static void inline cond_resched_rcu_lock(void) { rcu_read_unlock(); WARN_ON_ONCE(rcu_read_lock_held()); #ifndef CONFIG_PREEMPT_RCU cond_resched(); #endif rcu_read_lock(); } Of course, we could do this in your implementation as well: static void inline cond_resched_rcu_lock(void) { #ifdef CONFIG_PREEMPT_RCU if (need_resched()) { rcu_read_unlock(); WARN_ON_ONCE(rcu_read_lock_held()); cond_resched(); rcu_read_lock(); } #endif /* CONFIG_PREEMPT_RCU */ } But this would fail to detect the bug -- and would silently fail -- on !CONFIG_PREEMPT_RCU systems. Thanx, Paul > Of course, both approaches assume that the caller is in a place > where having all RCU-protected data disappear is OK! > > Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 14:32 ` Paul E. McKenney @ 2013-05-02 7:27 ` Peter Zijlstra 0 siblings, 0 replies; 37+ messages in thread From: Peter Zijlstra @ 2013-05-02 7:27 UTC (permalink / raw) To: Paul E. McKenney Cc: Julian Anastasov, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 07:32:58AM -0700, Paul E. McKenney wrote: > Here is how to detect this: > > static void inline cond_resched_rcu_lock(void) > { > rcu_read_unlock(); > WARN_ON_ONCE(rcu_read_lock_held()); > #ifndef CONFIG_PREEMPT_RCU > cond_resched(); > #endif > rcu_read_lock(); > } The __might_sleep(__FILE__, __LINE__, PREEMPT_RCU_OFFSET) I posted in the other email just now should deal with this. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 12:46 ` Paul E. McKenney 2013-05-01 14:32 ` Paul E. McKenney @ 2013-05-01 15:17 ` Peter Zijlstra 2013-05-01 15:29 ` Eric Dumazet 1 sibling, 1 reply; 37+ messages in thread From: Peter Zijlstra @ 2013-05-01 15:17 UTC (permalink / raw) To: Paul E. McKenney Cc: Julian Anastasov, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 05:46:37AM -0700, Paul E. McKenney wrote: > If the only goal is to allow preemption, and if long grace periods are > not a concern, then this alternate approach would work fine as well. Hmm.. if that were the goal I'd like it to have a different name; cond_resched*() has always been about preemption. > Of course, both approaches assume that the caller is in a place > where having all RCU-protected data disappear is OK! Quite :-) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 15:17 ` Peter Zijlstra @ 2013-05-01 15:29 ` Eric Dumazet 2013-05-01 15:59 ` Peter Zijlstra 2013-05-01 16:02 ` Paul E. McKenney 0 siblings, 2 replies; 37+ messages in thread From: Eric Dumazet @ 2013-05-01 15:29 UTC (permalink / raw) To: Peter Zijlstra Cc: Paul E. McKenney, Julian Anastasov, Simon Horman, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, 2013-05-01 at 17:17 +0200, Peter Zijlstra wrote: > On Wed, May 01, 2013 at 05:46:37AM -0700, Paul E. McKenney wrote: > > > If the only goal is to allow preemption, and if long grace periods are > > not a concern, then this alternate approach would work fine as well. > > Hmm.. if that were the goal I'd like it to have a different name; > cond_resched*() has always been about preemption. BTW, I do not remember why cond_resched() is not an empty macro when CONFIG_PREEMPT=y ? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 15:29 ` Eric Dumazet @ 2013-05-01 15:59 ` Peter Zijlstra 2013-05-01 16:02 ` Paul E. McKenney 1 sibling, 0 replies; 37+ messages in thread From: Peter Zijlstra @ 2013-05-01 15:59 UTC (permalink / raw) To: Eric Dumazet Cc: Paul E. McKenney, Julian Anastasov, Simon Horman, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 08:29:55AM -0700, Eric Dumazet wrote: > On Wed, 2013-05-01 at 17:17 +0200, Peter Zijlstra wrote: > > On Wed, May 01, 2013 at 05:46:37AM -0700, Paul E. McKenney wrote: > > > > > If the only goal is to allow preemption, and if long grace periods are > > > not a concern, then this alternate approach would work fine as well. > > > > Hmm.. if that were the goal I'd like it to have a different name; > > cond_resched*() has always been about preemption. > > BTW, I do not remember why cond_resched() is not an empty macro > when CONFIG_PREEMPT=y ? Good question.. at at least, only the __might_sleep() construct. Ingo, happen to remember why this is? Most of this infrastructure is from before my time. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 15:29 ` Eric Dumazet 2013-05-01 15:59 ` Peter Zijlstra @ 2013-05-01 16:02 ` Paul E. McKenney 2013-05-01 16:57 ` Peter Zijlstra 1 sibling, 1 reply; 37+ messages in thread From: Paul E. McKenney @ 2013-05-01 16:02 UTC (permalink / raw) To: Eric Dumazet Cc: Peter Zijlstra, Julian Anastasov, Simon Horman, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 08:29:55AM -0700, Eric Dumazet wrote: > On Wed, 2013-05-01 at 17:17 +0200, Peter Zijlstra wrote: > > On Wed, May 01, 2013 at 05:46:37AM -0700, Paul E. McKenney wrote: > > > > > If the only goal is to allow preemption, and if long grace periods are > > > not a concern, then this alternate approach would work fine as well. > > > > Hmm.. if that were the goal I'd like it to have a different name; > > cond_resched*() has always been about preemption. > > BTW, I do not remember why cond_resched() is not an empty macro > when CONFIG_PREEMPT=y ? My guess would be for the case where sched_preempt_enable_no_resched() is followed some time later by cond_resched(). Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 16:02 ` Paul E. McKenney @ 2013-05-01 16:57 ` Peter Zijlstra 2013-05-01 17:30 ` Paul E. McKenney 0 siblings, 1 reply; 37+ messages in thread From: Peter Zijlstra @ 2013-05-01 16:57 UTC (permalink / raw) To: Paul E. McKenney Cc: Eric Dumazet, Julian Anastasov, Simon Horman, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 09:02:18AM -0700, Paul E. McKenney wrote: > My guess would be for the case where sched_preempt_enable_no_resched() > is followed some time later by cond_resched(). I might hope not.. preempt_enable_no_resched() is nasty and you're likely to be hit with a frozen fish of sorts by tglx if you try to use it ;-) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 16:57 ` Peter Zijlstra @ 2013-05-01 17:30 ` Paul E. McKenney 0 siblings, 0 replies; 37+ messages in thread From: Paul E. McKenney @ 2013-05-01 17:30 UTC (permalink / raw) To: Peter Zijlstra Cc: Eric Dumazet, Julian Anastasov, Simon Horman, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 06:57:49PM +0200, Peter Zijlstra wrote: > On Wed, May 01, 2013 at 09:02:18AM -0700, Paul E. McKenney wrote: > > My guess would be for the case where sched_preempt_enable_no_resched() > > is followed some time later by cond_resched(). > > I might hope not.. preempt_enable_no_resched() is nasty and you're likely to be > hit with a frozen fish of sorts by tglx if you try to use it ;-) I will stick with my guess, though I agree that if I am correct, this situation almost certainly predates tglx's Linux-related use of frozen fish as projectile weapons. ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 9:10 ` Peter Zijlstra 2013-05-01 12:46 ` Paul E. McKenney @ 2013-05-01 14:22 ` Julian Anastasov 2013-05-01 15:55 ` Peter Zijlstra 1 sibling, 1 reply; 37+ messages in thread From: Julian Anastasov @ 2013-05-01 14:22 UTC (permalink / raw) To: Peter Zijlstra Cc: Simon Horman, Eric Dumazet, Ingo Molnar, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Wed, 1 May 2013, Peter Zijlstra wrote: > On Tue, Apr 30, 2013 at 10:52:38AM +0300, Julian Anastasov wrote: > > > > Hello, > > > > On Tue, 30 Apr 2013, Simon Horman wrote: > > > > > Thanks, to clarify, just this: > > > > > > static void inline cond_resched_rcu_lock(void) > > > { > > > rcu_read_unlock(); > > > #ifndef CONFIG_PREEMPT_RCU > > > cond_resched(); > > > #endif > > > rcu_read_lock(); > > > } > > > > Yes, thanks! > > OK, now I'm confused.. PREEMPT_RCU would preempt in any case, so why bother > dropping rcu_read_lock() at all? > > That is; the thing that makes sense to me is: > > static void inline cond_resched_rcu_lock(void) > { > #ifdef CONFIG_PREEMPT_RCU You mean '#ifndef' here, right? But in the non-preempt case is using the need_resched() needed? rcu_read_unlock and rcu_read_lock do not generate code. > if (need_resched()) { > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > } > #endif /* CONFIG_PREEMPT_RCU */ > } > > That would have an rcu_read_lock() break and voluntary preemption point for > non-preemptible RCU and not bother with the stuff for preemptible RCU. I see. So, can we choose one of both variants: 1. Your variant but with ifndef: static void inline cond_resched_rcu_lock(void) { #ifndef CONFIG_PREEMPT_RCU if (need_resched()) { rcu_read_unlock(); cond_resched(); rcu_read_lock(); } #endif } 2. Same without need_resched because cond_resched already performs the same checks: static void inline cond_resched_rcu_lock(void) { #ifndef CONFIG_PREEMPT_RCU rcu_read_unlock(); cond_resched(); rcu_read_lock(); #endif } Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 14:22 ` Julian Anastasov @ 2013-05-01 15:55 ` Peter Zijlstra 2013-05-01 18:22 ` Julian Anastasov 0 siblings, 1 reply; 37+ messages in thread From: Peter Zijlstra @ 2013-05-01 15:55 UTC (permalink / raw) To: Julian Anastasov Cc: Simon Horman, Eric Dumazet, Ingo Molnar, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 05:22:05PM +0300, Julian Anastasov wrote: > > static void inline cond_resched_rcu_lock(void) > > { > > #ifdef CONFIG_PREEMPT_RCU > > You mean '#ifndef' here, right? But in the non-preempt > case is using the need_resched() needed? rcu_read_unlock > and rcu_read_lock do not generate code. Uhm... yes! > > if (need_resched()) { > > rcu_read_unlock(); > > cond_resched(); > > rcu_read_lock(); > > } > > #endif /* CONFIG_PREEMPT_RCU */ > > } > > > > That would have an rcu_read_lock() break and voluntary preemption point for > > non-preemptible RCU and not bother with the stuff for preemptible RCU. > > I see. So, can we choose one of both variants: > > 1. Your variant but with ifndef: > > static void inline cond_resched_rcu_lock(void) > { > #ifndef CONFIG_PREEMPT_RCU > if (need_resched()) { > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > } > #endif > } > > 2. Same without need_resched because cond_resched already > performs the same checks: > > static void inline cond_resched_rcu_lock(void) > { > #ifndef CONFIG_PREEMPT_RCU > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > #endif > } Ah so the 'problem' with this last version is that it does an unconditional / unnessecary rcu_read_unlock(). The below would be in line with all the other cond_resched*() implementations. --- include/linux/sched.h | 7 +++++++ kernel/sched/core.c | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/include/linux/sched.h b/include/linux/sched.h index 802a751..fd2c77f 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -2449,6 +2449,13 @@ extern int __cond_resched_softirq(void); __cond_resched_softirq(); \ }) +extern int __cond_resched_rcu(void); + +#define cond_resched_rcu() ({ \ + __might_sleep(__FILE__, __LINE__, 0); \ + __cond_resched_rcu(); \ +}) + /* * Does a critical section need to be broken due to another * task waiting?: (technically does not depend on CONFIG_PREEMPT, diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 7d7901a..2b3b4e6 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4358,6 +4358,20 @@ int __sched __cond_resched_softirq(void) } EXPORT_SYMBOL(__cond_resched_softirq); +int __sched __cond_resched_rcu(void) +{ +#ifndef CONFIG_PREEMPT_RCU + if (should_resched()) { + rcu_read_unlock(); + __cond_resched(); + rcu_read_lock(); + return 1; + } +#endif + return 0; +} +EXPORT_SYMBOL(__cond_resched_rcu); + /** * yield - yield the current processor to other threads. * ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 15:55 ` Peter Zijlstra @ 2013-05-01 18:22 ` Julian Anastasov 2013-05-01 19:04 ` Paul E. McKenney 2013-05-02 7:26 ` Peter Zijlstra 0 siblings, 2 replies; 37+ messages in thread From: Julian Anastasov @ 2013-05-01 18:22 UTC (permalink / raw) To: Peter Zijlstra Cc: Simon Horman, Eric Dumazet, Ingo Molnar, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Wed, 1 May 2013, Peter Zijlstra wrote: > On Wed, May 01, 2013 at 05:22:05PM +0300, Julian Anastasov wrote: > > > 2. Same without need_resched because cond_resched already > > performs the same checks: > > > > static void inline cond_resched_rcu_lock(void) > > { > > #ifndef CONFIG_PREEMPT_RCU > > rcu_read_unlock(); > > cond_resched(); > > rcu_read_lock(); > > #endif > > } > > Ah so the 'problem' with this last version is that it does an unconditional / > unnessecary rcu_read_unlock(). It is just a barrier() for the non-preempt case. > The below would be in line with all the other cond_resched*() implementations. ... > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 802a751..fd2c77f 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -2449,6 +2449,13 @@ extern int __cond_resched_softirq(void); > __cond_resched_softirq(); \ > }) > > +extern int __cond_resched_rcu(void); > + > +#define cond_resched_rcu() ({ \ > + __might_sleep(__FILE__, __LINE__, 0); \ I see your goal. But digging into __might_sleep() I see that rcu_sleep_check() will scream for the non-preempt case because we are under rcu_read_lock. What about such inline version: static void inline cond_resched_rcu(void) { #ifndef CONFIG_PREEMPT_RCU rcu_read_unlock(); __might_sleep(__FILE__, __LINE__, 0); cond_resched(); rcu_read_lock(); #else __might_sleep(__FILE__, __LINE__, 0); rcu_lockdep_assert(rcu_preempt_depth() == 1, "Illegal cond_resched_rcu() context"); #endif } It will restrict to single RCU lock level for all RCU implementations. But we don't have _cond_resched_rcu helper for two reasons: - __might_sleep uses __FILE__, __LINE__ - only cond_resched generates code, so need_resched() is avoided > + __cond_resched_rcu(); \ > +}) > + > /* > * Does a critical section need to be broken due to another > * task waiting?: (technically does not depend on CONFIG_PREEMPT, > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 7d7901a..2b3b4e6 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -4358,6 +4358,20 @@ int __sched __cond_resched_softirq(void) > } > EXPORT_SYMBOL(__cond_resched_softirq); > > +int __sched __cond_resched_rcu(void) > +{ > +#ifndef CONFIG_PREEMPT_RCU > + if (should_resched()) { > + rcu_read_unlock(); > + __cond_resched(); > + rcu_read_lock(); > + return 1; > + } > +#endif > + return 0; > +} > +EXPORT_SYMBOL(__cond_resched_rcu); > + Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 18:22 ` Julian Anastasov @ 2013-05-01 19:04 ` Paul E. McKenney 2013-05-02 7:26 ` Peter Zijlstra 1 sibling, 0 replies; 37+ messages in thread From: Paul E. McKenney @ 2013-05-01 19:04 UTC (permalink / raw) To: Julian Anastasov Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 09:22:08PM +0300, Julian Anastasov wrote: > > Hello, > > On Wed, 1 May 2013, Peter Zijlstra wrote: > > > On Wed, May 01, 2013 at 05:22:05PM +0300, Julian Anastasov wrote: > > > > > 2. Same without need_resched because cond_resched already > > > performs the same checks: > > > > > > static void inline cond_resched_rcu_lock(void) > > > { > > > #ifndef CONFIG_PREEMPT_RCU > > > rcu_read_unlock(); > > > cond_resched(); > > > rcu_read_lock(); > > > #endif > > > } > > > > Ah so the 'problem' with this last version is that it does an unconditional / > > unnessecary rcu_read_unlock(). > > It is just a barrier() for the non-preempt case. > > > The below would be in line with all the other cond_resched*() implementations. > > ... > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index 802a751..fd2c77f 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -2449,6 +2449,13 @@ extern int __cond_resched_softirq(void); > > __cond_resched_softirq(); \ > > }) > > > > +extern int __cond_resched_rcu(void); > > + > > +#define cond_resched_rcu() ({ \ > > + __might_sleep(__FILE__, __LINE__, 0); \ > > I see your goal. But digging into __might_sleep() > I see that rcu_sleep_check() will scream for the non-preempt > case because we are under rcu_read_lock. > > What about such inline version: > > static void inline cond_resched_rcu(void) > { > #ifndef CONFIG_PREEMPT_RCU > rcu_read_unlock(); > __might_sleep(__FILE__, __LINE__, 0); > cond_resched(); > rcu_read_lock(); > #else > __might_sleep(__FILE__, __LINE__, 0); > rcu_lockdep_assert(rcu_preempt_depth() == 1, > "Illegal cond_resched_rcu() context"); The above requires that include/linux/sched.h be included. This might be OK, but please check the current intended uses. Thanx, Paul > #endif > } > > It will restrict to single RCU lock level for all > RCU implementations. But we don't have _cond_resched_rcu > helper for two reasons: > > - __might_sleep uses __FILE__, __LINE__ > - only cond_resched generates code, so need_resched() is > avoided > > > + __cond_resched_rcu(); \ > > +}) > > + > > /* > > * Does a critical section need to be broken due to another > > * task waiting?: (technically does not depend on CONFIG_PREEMPT, > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index 7d7901a..2b3b4e6 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -4358,6 +4358,20 @@ int __sched __cond_resched_softirq(void) > > } > > EXPORT_SYMBOL(__cond_resched_softirq); > > > > +int __sched __cond_resched_rcu(void) > > +{ > > +#ifndef CONFIG_PREEMPT_RCU > > + if (should_resched()) { > > + rcu_read_unlock(); > > + __cond_resched(); > > + rcu_read_lock(); > > + return 1; > > + } > > +#endif > > + return 0; > > +} > > +EXPORT_SYMBOL(__cond_resched_rcu); > > + > > Regards > > -- > Julian Anastasov <ja@ssi.bg> > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-01 18:22 ` Julian Anastasov 2013-05-01 19:04 ` Paul E. McKenney @ 2013-05-02 7:26 ` Peter Zijlstra 2013-05-02 10:06 ` Julian Anastasov 2013-05-02 15:54 ` Julian Anastasov 1 sibling, 2 replies; 37+ messages in thread From: Peter Zijlstra @ 2013-05-02 7:26 UTC (permalink / raw) To: Julian Anastasov Cc: Simon Horman, Eric Dumazet, Ingo Molnar, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Wed, May 01, 2013 at 09:22:08PM +0300, Julian Anastasov wrote: > > +extern int __cond_resched_rcu(void); > > + > > +#define cond_resched_rcu() ({ \ > > + __might_sleep(__FILE__, __LINE__, 0); \ > > I see your goal. But digging into __might_sleep() > I see that rcu_sleep_check() will scream for the non-preempt > case because we are under rcu_read_lock. #ifdef CONFIG_PREEMPT_RCU #define PREEMPT_RCU_OFFSET 0 #else #define PREEMPT_RCU_OFFSET 1 #endif #define cond_resched_rcu() ({ \ __might_sleep(__FILE__, __LINE__, PREEMPT_RCU_OFFSET); \ __cond_resched_rcu(); \ }) Should work I think.. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 7:26 ` Peter Zijlstra @ 2013-05-02 10:06 ` Julian Anastasov 2013-05-02 15:54 ` Julian Anastasov 1 sibling, 0 replies; 37+ messages in thread From: Julian Anastasov @ 2013-05-02 10:06 UTC (permalink / raw) To: Peter Zijlstra Cc: Simon Horman, Eric Dumazet, Ingo Molnar, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Thu, 2 May 2013, Peter Zijlstra wrote: > On Wed, May 01, 2013 at 09:22:08PM +0300, Julian Anastasov wrote: > > > +extern int __cond_resched_rcu(void); > > > + > > > +#define cond_resched_rcu() ({ \ > > > + __might_sleep(__FILE__, __LINE__, 0); \ > > > > I see your goal. But digging into __might_sleep() > > I see that rcu_sleep_check() will scream for the non-preempt > > case because we are under rcu_read_lock. > > > #ifdef CONFIG_PREEMPT_RCU > #define PREEMPT_RCU_OFFSET 0 > #else > #define PREEMPT_RCU_OFFSET 1 > #endif > > #define cond_resched_rcu() ({ \ > __might_sleep(__FILE__, __LINE__, PREEMPT_RCU_OFFSET); \ > __cond_resched_rcu(); \ > }) > > Should work I think.. Looks like CONFIG_DEBUG_ATOMIC_SLEEP selects CONFIG_PREEMPT_COUNT, so PREEMPT_RCU_OFFSET should be 1 in all cases because preempt_disable() adds 1, while for CONFIG_PREEMPT_RCU case rcu_preempt_depth() should return 1: #ifdef CONFIG_PREEMPT_RCU #define PREEMPT_RCU_OFFSET 1 #else #define PREEMPT_RCU_OFFSET PREEMPT_CHECK_OFFSET #endif Now the remaining part is to fix rcu_sleep_check() for the non-preempt case. As there are no nesting depths in this case, I don't see a solution so far. We can provide some argument to rcu_preempt_sleep_check to compare depth with preempt_count() but currently I don't know how to differentiate cond_resched_lock() from cond_resched_rcu() when calling __might_sleep, in both cases we provide PREEMPT_OFFSET. May be some trick is needed here without adding new arg to __might_sleep, so that we can properly check for rcu_lock_map. Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 7:26 ` Peter Zijlstra 2013-05-02 10:06 ` Julian Anastasov @ 2013-05-02 15:54 ` Julian Anastasov 2013-05-02 17:32 ` Paul E. McKenney 1 sibling, 1 reply; 37+ messages in thread From: Julian Anastasov @ 2013-05-02 15:54 UTC (permalink / raw) To: Peter Zijlstra Cc: Simon Horman, Eric Dumazet, Ingo Molnar, Paul E. McKenney, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Thu, 2 May 2013, Peter Zijlstra wrote: > On Wed, May 01, 2013 at 09:22:08PM +0300, Julian Anastasov wrote: > > > +extern int __cond_resched_rcu(void); > > > + > > > +#define cond_resched_rcu() ({ \ > > > + __might_sleep(__FILE__, __LINE__, 0); \ > > > > I see your goal. But digging into __might_sleep() > > I see that rcu_sleep_check() will scream for the non-preempt > > case because we are under rcu_read_lock. > > > #ifdef CONFIG_PREEMPT_RCU > #define PREEMPT_RCU_OFFSET 0 > #else > #define PREEMPT_RCU_OFFSET 1 > #endif > > #define cond_resched_rcu() ({ \ > __might_sleep(__FILE__, __LINE__, PREEMPT_RCU_OFFSET); \ > __cond_resched_rcu(); \ > }) > > Should work I think.. I implemented your idea. I tested the following patch in 2 variants, TINY_RCU and CONFIG_TREE_PREEMPT_RCU. I see the error if extra rcu_read_lock is added for testing. I'm using the PREEMPT_ACTIVE flag to indicate that we are already under lock. It should work because __might_sleep is not called with such bit. I also tried to add new flag in include/linux/hardirq.h but PREEMPT_ACTIVE depends on the arch, so this alternative looked difficult to implement. include/linux/rcupdate.h | 7 ++++--- include/linux/sched.h | 14 ++++++++++++++ kernel/sched/core.c | 20 ++++++++++++++++++-- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index b758ce1..b594759 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -480,9 +480,10 @@ static inline void rcu_preempt_sleep_check(void) } #endif /* #else #ifdef CONFIG_PROVE_RCU */ -#define rcu_sleep_check() \ +#define rcu_sleep_check(locked) \ do { \ - rcu_preempt_sleep_check(); \ + if (!(locked)) \ + rcu_preempt_sleep_check(); \ rcu_lockdep_assert(!lock_is_held(&rcu_bh_lock_map), \ "Illegal context switch in RCU-bh" \ " read-side critical section"); \ @@ -494,7 +495,7 @@ static inline void rcu_preempt_sleep_check(void) #else /* #ifdef CONFIG_PROVE_RCU */ #define rcu_lockdep_assert(c, s) do { } while (0) -#define rcu_sleep_check() do { } while (0) +#define rcu_sleep_check(locked) do { } while (0) #endif /* #else #ifdef CONFIG_PROVE_RCU */ diff --git a/include/linux/sched.h b/include/linux/sched.h index e692a02..027deea 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -2608,6 +2608,20 @@ extern int __cond_resched_softirq(void); __cond_resched_softirq(); \ }) +#ifdef CONFIG_PREEMPT_RCU +#define PREEMPT_RCU_OFFSET 1 +#else +#define PREEMPT_RCU_OFFSET PREEMPT_CHECK_OFFSET +#endif + +extern int __cond_resched_rcu(void); + +#define cond_resched_rcu() ({ \ + __might_sleep(__FILE__, __LINE__, PREEMPT_ACTIVE | \ + PREEMPT_RCU_OFFSET); \ + __cond_resched_rcu(); \ +}) + /* * Does a critical section need to be broken due to another * task waiting?: (technically does not depend on CONFIG_PREEMPT, diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 67d0465..2724be7 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -2793,7 +2793,7 @@ static inline void schedule_debug(struct task_struct *prev) */ if (unlikely(in_atomic_preempt_off() && !prev->exit_state)) __schedule_bug(prev); - rcu_sleep_check(); + rcu_sleep_check(0); profile_hit(SCHED_PROFILING, __builtin_return_address(0)); @@ -4364,6 +4364,20 @@ int __sched __cond_resched_softirq(void) } EXPORT_SYMBOL(__cond_resched_softirq); +int __sched __cond_resched_rcu(void) +{ +#ifndef CONFIG_PREEMPT_RCU + if (should_resched()) { + rcu_read_unlock(); + __cond_resched(); + rcu_read_lock(); + return 1; + } +#endif + return 0; +} +EXPORT_SYMBOL(__cond_resched_rcu); + /** * yield - yield the current processor to other threads. * @@ -7062,7 +7076,9 @@ void __might_sleep(const char *file, int line, int preempt_offset) { static unsigned long prev_jiffy; /* ratelimiting */ - rcu_sleep_check(); /* WARN_ON_ONCE() by default, no rate limit reqd. */ + /* WARN_ON_ONCE() by default, no rate limit reqd. */ + rcu_sleep_check(preempt_offset & PREEMPT_ACTIVE); + preempt_offset &= ~PREEMPT_ACTIVE; if ((preempt_count_equals(preempt_offset) && !irqs_disabled()) || system_state != SYSTEM_RUNNING || oops_in_progress) return; -- 1.7.3.4 Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 15:54 ` Julian Anastasov @ 2013-05-02 17:32 ` Paul E. McKenney 2013-05-02 18:55 ` Julian Anastasov 0 siblings, 1 reply; 37+ messages in thread From: Paul E. McKenney @ 2013-05-02 17:32 UTC (permalink / raw) To: Julian Anastasov Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Thu, May 02, 2013 at 06:54:05PM +0300, Julian Anastasov wrote: > > Hello, > > On Thu, 2 May 2013, Peter Zijlstra wrote: > > > On Wed, May 01, 2013 at 09:22:08PM +0300, Julian Anastasov wrote: > > > > +extern int __cond_resched_rcu(void); > > > > + > > > > +#define cond_resched_rcu() ({ \ > > > > + __might_sleep(__FILE__, __LINE__, 0); \ > > > > > > I see your goal. But digging into __might_sleep() > > > I see that rcu_sleep_check() will scream for the non-preempt > > > case because we are under rcu_read_lock. > > > > > > #ifdef CONFIG_PREEMPT_RCU > > #define PREEMPT_RCU_OFFSET 0 > > #else > > #define PREEMPT_RCU_OFFSET 1 > > #endif > > > > #define cond_resched_rcu() ({ \ > > __might_sleep(__FILE__, __LINE__, PREEMPT_RCU_OFFSET); \ > > __cond_resched_rcu(); \ > > }) > > > > Should work I think.. > > I implemented your idea. > > I tested the following patch in 2 variants, > TINY_RCU and CONFIG_TREE_PREEMPT_RCU. I see the Could you please also try CONFIG_TREE_RCU? > error if extra rcu_read_lock is added for testing. > > I'm using the PREEMPT_ACTIVE flag to indicate > that we are already under lock. It should work because > __might_sleep is not called with such bit. I also tried to > add new flag in include/linux/hardirq.h but PREEMPT_ACTIVE > depends on the arch, so this alternative looked difficult to > implement. > > include/linux/rcupdate.h | 7 ++++--- > include/linux/sched.h | 14 ++++++++++++++ > kernel/sched/core.c | 20 ++++++++++++++++++-- > 3 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index b758ce1..b594759 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -480,9 +480,10 @@ static inline void rcu_preempt_sleep_check(void) > } > #endif /* #else #ifdef CONFIG_PROVE_RCU */ > > -#define rcu_sleep_check() \ > +#define rcu_sleep_check(locked) \ > do { \ > - rcu_preempt_sleep_check(); \ > + if (!(locked)) \ > + rcu_preempt_sleep_check(); \ > rcu_lockdep_assert(!lock_is_held(&rcu_bh_lock_map), \ > "Illegal context switch in RCU-bh" \ > " read-side critical section"); \ > @@ -494,7 +495,7 @@ static inline void rcu_preempt_sleep_check(void) > #else /* #ifdef CONFIG_PROVE_RCU */ > > #define rcu_lockdep_assert(c, s) do { } while (0) > -#define rcu_sleep_check() do { } while (0) > +#define rcu_sleep_check(locked) do { } while (0) > > #endif /* #else #ifdef CONFIG_PROVE_RCU */ > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index e692a02..027deea 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -2608,6 +2608,20 @@ extern int __cond_resched_softirq(void); > __cond_resched_softirq(); \ > }) > > +#ifdef CONFIG_PREEMPT_RCU > +#define PREEMPT_RCU_OFFSET 1 > +#else > +#define PREEMPT_RCU_OFFSET PREEMPT_CHECK_OFFSET > +#endif > + > +extern int __cond_resched_rcu(void); > + > +#define cond_resched_rcu() ({ \ > + __might_sleep(__FILE__, __LINE__, PREEMPT_ACTIVE | \ > + PREEMPT_RCU_OFFSET); \ > + __cond_resched_rcu(); \ > +}) > + > /* > * Does a critical section need to be broken due to another > * task waiting?: (technically does not depend on CONFIG_PREEMPT, > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 67d0465..2724be7 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2793,7 +2793,7 @@ static inline void schedule_debug(struct task_struct *prev) > */ > if (unlikely(in_atomic_preempt_off() && !prev->exit_state)) > __schedule_bug(prev); > - rcu_sleep_check(); > + rcu_sleep_check(0); > > profile_hit(SCHED_PROFILING, __builtin_return_address(0)); > > @@ -4364,6 +4364,20 @@ int __sched __cond_resched_softirq(void) > } > EXPORT_SYMBOL(__cond_resched_softirq); > > +int __sched __cond_resched_rcu(void) > +{ > +#ifndef CONFIG_PREEMPT_RCU > + if (should_resched()) { > + rcu_read_unlock(); > + __cond_resched(); > + rcu_read_lock(); > + return 1; > + } > +#endif > + return 0; > +} > +EXPORT_SYMBOL(__cond_resched_rcu); > + > /** > * yield - yield the current processor to other threads. > * > @@ -7062,7 +7076,9 @@ void __might_sleep(const char *file, int line, int preempt_offset) > { > static unsigned long prev_jiffy; /* ratelimiting */ > > - rcu_sleep_check(); /* WARN_ON_ONCE() by default, no rate limit reqd. */ > + /* WARN_ON_ONCE() by default, no rate limit reqd. */ > + rcu_sleep_check(preempt_offset & PREEMPT_ACTIVE); Color me confused. >From what I can see, the two values passed in through preempt_offset are PREEMPT_LOCK_OFFSET and SOFTIRQ_DISABLE_OFFSET. PREEMPT_ACTIVE is normally a high-order bit, above PREEMPT_MASK, SOFTIRQ_MASK, and HARDIRQ_MASK. PREEMPT_LOCK_OFFSET and SOFTIRQ_DISABLE_OFFSET have only low-order bits, so I don't see how rcu_sleep_check() is passed anything other than zero. Am I going blind, or what? Thanx, Paul > + preempt_offset &= ~PREEMPT_ACTIVE; > if ((preempt_count_equals(preempt_offset) && !irqs_disabled()) || > system_state != SYSTEM_RUNNING || oops_in_progress) > return; > -- > 1.7.3.4 > > > Regards > > -- > Julian Anastasov <ja@ssi.bg> > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 17:32 ` Paul E. McKenney @ 2013-05-02 18:55 ` Julian Anastasov 2013-05-02 19:24 ` Julian Anastasov 2013-05-02 19:34 ` Paul E. McKenney 0 siblings, 2 replies; 37+ messages in thread From: Julian Anastasov @ 2013-05-02 18:55 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Thu, 2 May 2013, Paul E. McKenney wrote: > On Thu, May 02, 2013 at 06:54:05PM +0300, Julian Anastasov wrote: > > > > I tested the following patch in 2 variants, > > TINY_RCU and CONFIG_TREE_PREEMPT_RCU. I see the > > Could you please also try CONFIG_TREE_RCU? Note that I'm testing on some 9-year old UP system, i.e. 1 CPU. Now I enabled SMP to test CONFIG_TREE_RCU and the results are same. I think, it should be just like the TINY_RCU in terms of these debuggings (non-preempt). Extra rcu_read_lock gives me "Illegal context switch in RCU read-side critical section" in addition to the "BUG: sleeping function called from invalid context" message. > > error if extra rcu_read_lock is added for testing. > > > > I'm using the PREEMPT_ACTIVE flag to indicate > > that we are already under lock. It should work because > > __might_sleep is not called with such bit. I also tried to > > add new flag in include/linux/hardirq.h but PREEMPT_ACTIVE > > depends on the arch, so this alternative looked difficult to > > implement. > > +extern int __cond_resched_rcu(void); > > + > > +#define cond_resched_rcu() ({ \ > > + __might_sleep(__FILE__, __LINE__, PREEMPT_ACTIVE | \ > > + PREEMPT_RCU_OFFSET); \ > > + __cond_resched_rcu(); \ > > +}) > > + > > @@ -7062,7 +7076,9 @@ void __might_sleep(const char *file, int line, int preempt_offset) > > { > > static unsigned long prev_jiffy; /* ratelimiting */ > > > > - rcu_sleep_check(); /* WARN_ON_ONCE() by default, no rate limit reqd. */ > > + /* WARN_ON_ONCE() by default, no rate limit reqd. */ > > + rcu_sleep_check(preempt_offset & PREEMPT_ACTIVE); > > Color me confused. > > >From what I can see, the two values passed in through preempt_offset > are PREEMPT_LOCK_OFFSET and SOFTIRQ_DISABLE_OFFSET. PREEMPT_ACTIVE > is normally a high-order bit, above PREEMPT_MASK, SOFTIRQ_MASK, and > HARDIRQ_MASK. > > PREEMPT_LOCK_OFFSET and SOFTIRQ_DISABLE_OFFSET have only low-order bits, > so I don't see how rcu_sleep_check() is passed anything other than zero. > Am I going blind, or what? Only the new cond_resched_rcu() macro provides PREEMPT_ACTIVE flag to skip the rcu_preempt_sleep_check() call. The old macros provide locked=0 as you noticed. Does it answer your question or I'm missing something? Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 18:55 ` Julian Anastasov @ 2013-05-02 19:24 ` Julian Anastasov 2013-05-02 19:34 ` Paul E. McKenney 1 sibling, 0 replies; 37+ messages in thread From: Julian Anastasov @ 2013-05-02 19:24 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Thu, 2 May 2013, Julian Anastasov wrote: > Note that I'm testing on some 9-year old > UP system, i.e. 1 CPU. Now I enabled SMP to test CONFIG_TREE_RCU > and the results are same. I think, it should be just like > the TINY_RCU in terms of these debuggings (non-preempt). Extra > rcu_read_lock gives me "Illegal context switch in RCU read-side > critical section" in addition to the "BUG: sleeping function > called from invalid context" message. Just to clarify about the test with extra rcu_read_lock because above paragraph is very confusing: - The __might_sleep call with PREEMPT_ACTIVE | PREEMPT_RCU_OFFSET just warns with "BUG: sleeping function called from invalid context" because its rcu_sleep_check is silenced. We match the nesting depth only. - but __cond_resched -> __schedule -> schedule_debug warns about the extra rcu_read_lock() with "BUG: scheduling while atomic" and then with "Illegal context switch in RCU read-side critical section" from rcu_sleep_check(0). Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 18:55 ` Julian Anastasov 2013-05-02 19:24 ` Julian Anastasov @ 2013-05-02 19:34 ` Paul E. McKenney 2013-05-02 20:19 ` Julian Anastasov 1 sibling, 1 reply; 37+ messages in thread From: Paul E. McKenney @ 2013-05-02 19:34 UTC (permalink / raw) To: Julian Anastasov Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Thu, May 02, 2013 at 09:55:54PM +0300, Julian Anastasov wrote: > > Hello, > > On Thu, 2 May 2013, Paul E. McKenney wrote: > > > On Thu, May 02, 2013 at 06:54:05PM +0300, Julian Anastasov wrote: > > > > > > I tested the following patch in 2 variants, > > > TINY_RCU and CONFIG_TREE_PREEMPT_RCU. I see the > > > > Could you please also try CONFIG_TREE_RCU? > > Note that I'm testing on some 9-year old > UP system, i.e. 1 CPU. Now I enabled SMP to test CONFIG_TREE_RCU > and the results are same. I think, it should be just like > the TINY_RCU in terms of these debuggings (non-preempt). Extra > rcu_read_lock gives me "Illegal context switch in RCU read-side > critical section" in addition to the "BUG: sleeping function > called from invalid context" message. OK... > > > error if extra rcu_read_lock is added for testing. > > > > > > I'm using the PREEMPT_ACTIVE flag to indicate > > > that we are already under lock. It should work because > > > __might_sleep is not called with such bit. I also tried to > > > add new flag in include/linux/hardirq.h but PREEMPT_ACTIVE > > > depends on the arch, so this alternative looked difficult to > > > implement. > > > > +extern int __cond_resched_rcu(void); > > > + > > > +#define cond_resched_rcu() ({ \ > > > + __might_sleep(__FILE__, __LINE__, PREEMPT_ACTIVE | \ > > > + PREEMPT_RCU_OFFSET); \ > > > + __cond_resched_rcu(); \ > > > +}) > > > + > > > > @@ -7062,7 +7076,9 @@ void __might_sleep(const char *file, int line, int preempt_offset) > > > { > > > static unsigned long prev_jiffy; /* ratelimiting */ > > > > > > - rcu_sleep_check(); /* WARN_ON_ONCE() by default, no rate limit reqd. */ > > > + /* WARN_ON_ONCE() by default, no rate limit reqd. */ > > > + rcu_sleep_check(preempt_offset & PREEMPT_ACTIVE); > > > > Color me confused. > > > > >From what I can see, the two values passed in through preempt_offset > > are PREEMPT_LOCK_OFFSET and SOFTIRQ_DISABLE_OFFSET. PREEMPT_ACTIVE > > is normally a high-order bit, above PREEMPT_MASK, SOFTIRQ_MASK, and > > HARDIRQ_MASK. > > > > PREEMPT_LOCK_OFFSET and SOFTIRQ_DISABLE_OFFSET have only low-order bits, > > so I don't see how rcu_sleep_check() is passed anything other than zero. > > Am I going blind, or what? > > Only the new cond_resched_rcu() macro provides > PREEMPT_ACTIVE flag to skip the rcu_preempt_sleep_check() > call. The old macros provide locked=0 as you noticed. Does it > answer your question or I'm missing something? PREEMPT_ACTIVE's value is usually 0x10000000. Did it change since 3.9? If not, rcu_sleep_check(preempt_offset & PREEMPT_ACTIVE) is the same as rcu_sleep_check(0). Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 19:34 ` Paul E. McKenney @ 2013-05-02 20:19 ` Julian Anastasov 2013-05-02 22:31 ` Paul E. McKenney 0 siblings, 1 reply; 37+ messages in thread From: Julian Anastasov @ 2013-05-02 20:19 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Thu, 2 May 2013, Paul E. McKenney wrote: > > Only the new cond_resched_rcu() macro provides > > PREEMPT_ACTIVE flag to skip the rcu_preempt_sleep_check() > > call. The old macros provide locked=0 as you noticed. Does it > > answer your question or I'm missing something? > > PREEMPT_ACTIVE's value is usually 0x10000000. Did it change > since 3.9? If not, rcu_sleep_check(preempt_offset & PREEMPT_ACTIVE) > is the same as rcu_sleep_check(0). Yes, the different platforms use different bit, that is why I mentioned about my failed attempt at changing hardirq.h. PREEMPT_ACTIVE is always != 0. But I don't understand what do you mean by 'preempt_offset & PREEMPT_ACTIVE' being always 0. It is always 0 for cond_resched(), cond_resched_lock() and cond_resched_softirq(), not for cond_resched_rcu(): (PREEMPT_ACTIVE | PREEMPT_RCU_OFFSET) & PREEMPT_ACTIVE should give PREEMPT_ACTIVE, not 0. We have 2 cases in rcu_sleep_check() for the if: 1. !(PREEMPT_ACTIVE) => FALSE for cond_resched_rcu 2. !(0) => TRUE for other cond_resched_* macros On x86 the code is: __might_sleep: pushl %ebp # testl $268435456, %ecx #, preempt_offset ... jne .L240 #, // rcu_lock_map checked when PREEMPT_ACTIVE is missing .L240: // rcu_bh_lock_map checked Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 20:19 ` Julian Anastasov @ 2013-05-02 22:31 ` Paul E. McKenney 2013-05-03 7:52 ` Julian Anastasov 0 siblings, 1 reply; 37+ messages in thread From: Paul E. McKenney @ 2013-05-02 22:31 UTC (permalink / raw) To: Julian Anastasov Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Thu, May 02, 2013 at 11:19:12PM +0300, Julian Anastasov wrote: > > Hello, > > On Thu, 2 May 2013, Paul E. McKenney wrote: > > > > Only the new cond_resched_rcu() macro provides > > > PREEMPT_ACTIVE flag to skip the rcu_preempt_sleep_check() > > > call. The old macros provide locked=0 as you noticed. Does it > > > answer your question or I'm missing something? > > > > PREEMPT_ACTIVE's value is usually 0x10000000. Did it change > > since 3.9? If not, rcu_sleep_check(preempt_offset & PREEMPT_ACTIVE) > > is the same as rcu_sleep_check(0). > > Yes, the different platforms use different bit, > that is why I mentioned about my failed attempt at > changing hardirq.h. PREEMPT_ACTIVE is always != 0. > > But I don't understand what do you mean by > 'preempt_offset & PREEMPT_ACTIVE' being always 0. > It is always 0 for cond_resched(), cond_resched_lock() > and cond_resched_softirq(), not for cond_resched_rcu(): > > (PREEMPT_ACTIVE | PREEMPT_RCU_OFFSET) & PREEMPT_ACTIVE > should give PREEMPT_ACTIVE, not 0. We have 2 cases in > rcu_sleep_check() for the if: > > 1. !(PREEMPT_ACTIVE) => FALSE for cond_resched_rcu > 2. !(0) => TRUE for other cond_resched_* macros > > On x86 the code is: > > __might_sleep: > pushl %ebp # > testl $268435456, %ecx #, preempt_offset > ... > jne .L240 #, > // rcu_lock_map checked when PREEMPT_ACTIVE is missing > .L240: > // rcu_bh_lock_map checked OK, apologies -- I was looking at the calls to __might_sleep() in mainline, and missed the one that you added. Revisiting that, a question: > +#ifdef CONFIG_PREEMPT_RCU > +#define PREEMPT_RCU_OFFSET 1 Does this really want to be "1" instead of PREEMPT_OFFSET? > +#else > +#define PREEMPT_RCU_OFFSET PREEMPT_CHECK_OFFSET > +#endif > + > +extern int __cond_resched_rcu(void); > + > +#define cond_resched_rcu() ({ \ > + __might_sleep(__FILE__, __LINE__, PREEMPT_ACTIVE | \ > + PREEMPT_RCU_OFFSET); \ > + __cond_resched_rcu(); \ > +}) > + For the rest, I clearly need to revisit when more alert, because right now I am not seeing the connection to preemptible RCU's rcu_read_lock() implementation. Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-02 22:31 ` Paul E. McKenney @ 2013-05-03 7:52 ` Julian Anastasov 2013-05-03 16:30 ` Paul E. McKenney 0 siblings, 1 reply; 37+ messages in thread From: Julian Anastasov @ 2013-05-03 7:52 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Thu, 2 May 2013, Paul E. McKenney wrote: > mainline, and missed the one that you added. Revisiting that, a > question: > > > +#ifdef CONFIG_PREEMPT_RCU > > +#define PREEMPT_RCU_OFFSET 1 > > Does this really want to be "1" instead of PREEMPT_OFFSET? In this case when CONFIG_PREEMPT_RCU is enabled we (RCU) do not touch the preempt counters. Instead, the units are accounted in current->rcu_read_lock_nesting: #define rcu_preempt_depth() (current->rcu_read_lock_nesting) __rcu_read_lock: current->rcu_read_lock_nesting++; and the path is __might_sleep -> preempt_count_equals -> rcu_preempt_depth For now both places do not use PREEMPT_OFFSET: - #define inc_preempt_count() add_preempt_count(1) - __rcu_read_lock: current->rcu_read_lock_nesting++; so, ... it does not matter much for me. In short, the trick is in preempt_count_equals() where preempt_offset is a combination of preempt count and RCU preempt depth: #ifdef CONFIG_PREEMPT_RCU #define PREEMPT_RCU_OFFSET (0 /* preempt */ + 1 /* RCU */) #else #define PREEMPT_RCU_OFFSET (PREEMPT_CHECK_OFFSET + 0 /* RCU */) #endif Let me know for your preference about this definition... Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-03 7:52 ` Julian Anastasov @ 2013-05-03 16:30 ` Paul E. McKenney 2013-05-03 17:04 ` Peter Zijlstra ` (2 more replies) 0 siblings, 3 replies; 37+ messages in thread From: Paul E. McKenney @ 2013-05-03 16:30 UTC (permalink / raw) To: Julian Anastasov Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Fri, May 03, 2013 at 10:52:36AM +0300, Julian Anastasov wrote: > > Hello, > > On Thu, 2 May 2013, Paul E. McKenney wrote: > > > mainline, and missed the one that you added. Revisiting that, a > > question: > > > > > +#ifdef CONFIG_PREEMPT_RCU > > > +#define PREEMPT_RCU_OFFSET 1 > > > > Does this really want to be "1" instead of PREEMPT_OFFSET? > > In this case when CONFIG_PREEMPT_RCU is enabled > we (RCU) do not touch the preempt counters. Instead, the units > are accounted in current->rcu_read_lock_nesting: > > #define rcu_preempt_depth() (current->rcu_read_lock_nesting) > > __rcu_read_lock: > current->rcu_read_lock_nesting++; > > and the path is __might_sleep -> preempt_count_equals -> > rcu_preempt_depth > > For now both places do not use PREEMPT_OFFSET: > > - #define inc_preempt_count() add_preempt_count(1) > - __rcu_read_lock: current->rcu_read_lock_nesting++; > > so, ... it does not matter much for me. In short, > the trick is in preempt_count_equals() where preempt_offset > is a combination of preempt count and RCU preempt depth: > > #ifdef CONFIG_PREEMPT_RCU > #define PREEMPT_RCU_OFFSET (0 /* preempt */ + 1 /* RCU */) > #else > #define PREEMPT_RCU_OFFSET (PREEMPT_CHECK_OFFSET + 0 /* RCU */) > #endif > > Let me know for your preference about this definition... OK, after getting some sleep, I might have located the root cause of my confusion yesterday. The key point is that I don't understand why we cannot get the effect we are looking for with the following in sched.h (or wherever): static inline int cond_resched_rcu(void) { #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) || !defined(CONFIG_PREEMPT_RCU) rcu_read_unlock(); cond_resched(); rcu_read_lock(); #endif } This adds absolutely no overhead in non-debug builds of CONFIG_PREEMPT_RCU, does the checking in debug builds, and allows voluntary preemption in !CONFIG_PREEMPT_RCU builds. CONFIG_PROVE_RCU builds will check for an (illegal) outer rcu_read_lock() in CONFIG_PREEMPT_RCU builds, and you will get "scheduling while atomic" in response to an outer rcu_read_lock() in !CONFIG_PREEMPT_RCU builds. It also seems to me a lot simpler. Does this work, or am I still missing something? Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-03 16:30 ` Paul E. McKenney @ 2013-05-03 17:04 ` Peter Zijlstra 2013-05-03 17:34 ` Paul E. McKenney 2013-05-03 17:47 ` Julian Anastasov 2013-05-04 7:23 ` Julian Anastasov 2 siblings, 1 reply; 37+ messages in thread From: Peter Zijlstra @ 2013-05-03 17:04 UTC (permalink / raw) To: Paul E. McKenney Cc: Julian Anastasov, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma > The key point is that I don't understand why we cannot get the effect > we are looking for with the following in sched.h (or wherever): > > static inline int cond_resched_rcu(void) > { > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) || !defined(CONFIG_PREEMPT_RCU) > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > #endif > } > > This adds absolutely no overhead in non-debug builds of CONFIG_PREEMPT_RCU, > does the checking in debug builds, and allows voluntary preemption in > !CONFIG_PREEMPT_RCU builds. CONFIG_PROVE_RCU builds will check for an > (illegal) outer rcu_read_lock() in CONFIG_PREEMPT_RCU builds, and you > will get "scheduling while atomic" in response to an outer rcu_read_lock() > in !CONFIG_PREEMPT_RCU builds. > > It also seems to me a lot simpler. > > Does this work, or am I still missing something? It can do quite a number of superfluous rcu_read_unlock()/lock() pairs for voluntary preemption kernels? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-03 17:04 ` Peter Zijlstra @ 2013-05-03 17:34 ` Paul E. McKenney 2013-05-03 18:09 ` Peter Zijlstra 0 siblings, 1 reply; 37+ messages in thread From: Paul E. McKenney @ 2013-05-03 17:34 UTC (permalink / raw) To: Peter Zijlstra Cc: Julian Anastasov, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Fri, May 03, 2013 at 07:04:47PM +0200, Peter Zijlstra wrote: > > The key point is that I don't understand why we cannot get the effect > > we are looking for with the following in sched.h (or wherever): > > > > static inline int cond_resched_rcu(void) > > { > > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) || !defined(CONFIG_PREEMPT_RCU) > > rcu_read_unlock(); > > cond_resched(); > > rcu_read_lock(); > > #endif > > } > > > > This adds absolutely no overhead in non-debug builds of CONFIG_PREEMPT_RCU, > > does the checking in debug builds, and allows voluntary preemption in > > !CONFIG_PREEMPT_RCU builds. CONFIG_PROVE_RCU builds will check for an > > (illegal) outer rcu_read_lock() in CONFIG_PREEMPT_RCU builds, and you > > will get "scheduling while atomic" in response to an outer rcu_read_lock() > > in !CONFIG_PREEMPT_RCU builds. > > > > It also seems to me a lot simpler. > > > > Does this work, or am I still missing something? > > It can do quite a number of superfluous rcu_read_unlock()/lock() pairs for > voluntary preemption kernels? This happens in only two cases: 1. CONFIG_PREEMPT_RCU=n kernels. But in this case, rcu_read_unlock() and rcu_read_lock() are free, at least for CONFIG_PROVE_LOCKING=n kernels. And if you have CONFIG_PROVE_LOCKING=y, any contribution from rcu_read_unlock() and rcu_read_lock() will be in the noise. 2. CONFIG_DEBUG_ATOMIC_SLEEP=y kernels -- but in this case, you -want- the debugging. So either the overhead is non-existent, or you explicitly asked for the resulting debugging. In particular, CONFIG_PREEMPT_RCU=y kernels have an empty static inline function, which is free -- unless CONFIG_DEBUG_ATOMIC_SLEEP=y, in which case you again explicitly asked for the debugging. So I do not believe that the extra rcu_read_unlock()/lock() pairs are a problem in any of the possible combinations of configurations. Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-03 17:34 ` Paul E. McKenney @ 2013-05-03 18:09 ` Peter Zijlstra 0 siblings, 0 replies; 37+ messages in thread From: Peter Zijlstra @ 2013-05-03 18:09 UTC (permalink / raw) To: Paul E. McKenney Cc: Julian Anastasov, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma > This happens in only two cases: > > 1. CONFIG_PREEMPT_RCU=n kernels. But in this case, rcu_read_unlock() > and rcu_read_lock() are free, at least for CONFIG_PROVE_LOCKING=n > kernels. And if you have CONFIG_PROVE_LOCKING=y, any contribution > from rcu_read_unlock() and rcu_read_lock() will be in the noise. Oh argh.. I completely overlooked that rcu_read_{,un}lock() are NOPs for PREEMPT=n kernels. /me crawls back under his stone.. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-03 16:30 ` Paul E. McKenney 2013-05-03 17:04 ` Peter Zijlstra @ 2013-05-03 17:47 ` Julian Anastasov 2013-05-04 7:23 ` Julian Anastasov 2 siblings, 0 replies; 37+ messages in thread From: Julian Anastasov @ 2013-05-03 17:47 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Fri, 3 May 2013, Paul E. McKenney wrote: > OK, after getting some sleep, I might have located the root cause of > my confusion yesterday. > > The key point is that I don't understand why we cannot get the effect > we are looking for with the following in sched.h (or wherever): > > static inline int cond_resched_rcu(void) > { > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) || !defined(CONFIG_PREEMPT_RCU) > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > #endif > } > > This adds absolutely no overhead in non-debug builds of CONFIG_PREEMPT_RCU, > does the checking in debug builds, and allows voluntary preemption in > !CONFIG_PREEMPT_RCU builds. CONFIG_PROVE_RCU builds will check for an > (illegal) outer rcu_read_lock() in CONFIG_PREEMPT_RCU builds, and you > will get "scheduling while atomic" in response to an outer rcu_read_lock() > in !CONFIG_PREEMPT_RCU builds. > > It also seems to me a lot simpler. > > Does this work, or am I still missing something? It should work. It is a better version of the 2nd variant I mentioned here: http://marc.info/?l=linux-netdev&m=136741839021257&w=2 I'll stick to this version, hope Peter Zijlstra agrees. Playing with PREEMPT_ACTIVE or another bit makes the things more complex. To summarize: - CONFIG_PREEMPT_RCU: - no empty functions called - CONFIG_DEBUG_ATOMIC_SLEEP can catch errors even for this case - non-CONFIG_PREEMPT_RCU: - rcu_read_lock and rcu_read_unlock are barrier(), so it expands just to cond_resched() I'll repeat the tests tomorrow and if there are no problems will post official version after the merge window. Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-03 16:30 ` Paul E. McKenney 2013-05-03 17:04 ` Peter Zijlstra 2013-05-03 17:47 ` Julian Anastasov @ 2013-05-04 7:23 ` Julian Anastasov 2013-05-04 18:03 ` Paul E. McKenney 2 siblings, 1 reply; 37+ messages in thread From: Julian Anastasov @ 2013-05-04 7:23 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma Hello, On Fri, 3 May 2013, Paul E. McKenney wrote: > static inline int cond_resched_rcu(void) > { > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) || !defined(CONFIG_PREEMPT_RCU) > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > #endif > } > > This adds absolutely no overhead in non-debug builds of CONFIG_PREEMPT_RCU, > does the checking in debug builds, and allows voluntary preemption in > !CONFIG_PREEMPT_RCU builds. CONFIG_PROVE_RCU builds will check for an > (illegal) outer rcu_read_lock() in CONFIG_PREEMPT_RCU builds, and you > will get "scheduling while atomic" in response to an outer rcu_read_lock() > in !CONFIG_PREEMPT_RCU builds. > > It also seems to me a lot simpler. > > Does this work, or am I still missing something? Works perfectly. Thanks! Tested CONFIG_PREEMPT_RCU=y/n, the errors messages with extra RCU lock. Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper 2013-05-04 7:23 ` Julian Anastasov @ 2013-05-04 18:03 ` Paul E. McKenney 0 siblings, 0 replies; 37+ messages in thread From: Paul E. McKenney @ 2013-05-04 18:03 UTC (permalink / raw) To: Julian Anastasov Cc: Peter Zijlstra, Simon Horman, Eric Dumazet, Ingo Molnar, lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma On Sat, May 04, 2013 at 10:23:31AM +0300, Julian Anastasov wrote: > > Hello, > > On Fri, 3 May 2013, Paul E. McKenney wrote: > > > static inline int cond_resched_rcu(void) > > { > > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) || !defined(CONFIG_PREEMPT_RCU) > > rcu_read_unlock(); > > cond_resched(); > > rcu_read_lock(); > > #endif > > } > > > > This adds absolutely no overhead in non-debug builds of CONFIG_PREEMPT_RCU, > > does the checking in debug builds, and allows voluntary preemption in > > !CONFIG_PREEMPT_RCU builds. CONFIG_PROVE_RCU builds will check for an > > (illegal) outer rcu_read_lock() in CONFIG_PREEMPT_RCU builds, and you > > will get "scheduling while atomic" in response to an outer rcu_read_lock() > > in !CONFIG_PREEMPT_RCU builds. > > > > It also seems to me a lot simpler. > > > > Does this work, or am I still missing something? > > Works perfectly. Thanks! Tested CONFIG_PREEMPT_RCU=y/n, > the errors messages with extra RCU lock. Very good! Please send the patch along, and I will ack it. Thanx, Paul ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 2/2] ipvs: Use cond_resched_rcu_lock() helper when dumping connections 2013-04-30 2:52 [PATCH v2 0/2] sched: Add cond_resched_rcu_lock() helper Simon Horman 2013-04-30 2:52 ` [PATCH v2 1/2] " Simon Horman @ 2013-04-30 2:52 ` Simon Horman 1 sibling, 0 replies; 37+ messages in thread From: Simon Horman @ 2013-04-30 2:52 UTC (permalink / raw) To: Eric Dumazet, Julian Anastasov, Ingo Molnar, Peter Zijlstra, Paul E. McKenney Cc: lvs-devel, netdev, netfilter-devel, linux-kernel, Pablo Neira Ayuso, Dipankar Sarma, Simon Horman, Ingo Molnar This avoids the situation where a dump of a large number of connections may prevent scheduling for a long time while also avoiding excessive calls to rcu_read_unlock() and rcu_read_lock(). Note that in the case of !CONFIG_PREEMPT_RCU this will add a call to cond_resched(). Compile tested only. Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Julian Anastasov <ja@ssi.bg> Cc: Ingo Molnar <mingo@kernel.org> Signed-off-by: Simon Horman <horms@verge.net.au> --- net/netfilter/ipvs/ip_vs_conn.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c index a083bda..42a7b33 100644 --- a/net/netfilter/ipvs/ip_vs_conn.c +++ b/net/netfilter/ipvs/ip_vs_conn.c @@ -975,8 +975,7 @@ static void *ip_vs_conn_array(struct seq_file *seq, loff_t pos) return cp; } } - rcu_read_unlock(); - rcu_read_lock(); + cond_resched_rcu_lock(); } return NULL; @@ -1015,8 +1014,7 @@ static void *ip_vs_conn_seq_next(struct seq_file *seq, void *v, loff_t *pos) iter->l = &ip_vs_conn_tab[idx]; return cp; } - rcu_read_unlock(); - rcu_read_lock(); + cond_resched_rcu_lock(); } iter->l = NULL; return NULL; -- 1.8.2.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
end of thread, other threads:[~2013-05-04 18:03 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-04-30 2:52 [PATCH v2 0/2] sched: Add cond_resched_rcu_lock() helper Simon Horman 2013-04-30 2:52 ` [PATCH v2 1/2] " Simon Horman 2013-04-30 7:12 ` Julian Anastasov 2013-04-30 7:29 ` Simon Horman 2013-04-30 7:52 ` Julian Anastasov 2013-05-01 9:10 ` Peter Zijlstra 2013-05-01 12:46 ` Paul E. McKenney 2013-05-01 14:32 ` Paul E. McKenney 2013-05-02 7:27 ` Peter Zijlstra 2013-05-01 15:17 ` Peter Zijlstra 2013-05-01 15:29 ` Eric Dumazet 2013-05-01 15:59 ` Peter Zijlstra 2013-05-01 16:02 ` Paul E. McKenney 2013-05-01 16:57 ` Peter Zijlstra 2013-05-01 17:30 ` Paul E. McKenney 2013-05-01 14:22 ` Julian Anastasov 2013-05-01 15:55 ` Peter Zijlstra 2013-05-01 18:22 ` Julian Anastasov 2013-05-01 19:04 ` Paul E. McKenney 2013-05-02 7:26 ` Peter Zijlstra 2013-05-02 10:06 ` Julian Anastasov 2013-05-02 15:54 ` Julian Anastasov 2013-05-02 17:32 ` Paul E. McKenney 2013-05-02 18:55 ` Julian Anastasov 2013-05-02 19:24 ` Julian Anastasov 2013-05-02 19:34 ` Paul E. McKenney 2013-05-02 20:19 ` Julian Anastasov 2013-05-02 22:31 ` Paul E. McKenney 2013-05-03 7:52 ` Julian Anastasov 2013-05-03 16:30 ` Paul E. McKenney 2013-05-03 17:04 ` Peter Zijlstra 2013-05-03 17:34 ` Paul E. McKenney 2013-05-03 18:09 ` Peter Zijlstra 2013-05-03 17:47 ` Julian Anastasov 2013-05-04 7:23 ` Julian Anastasov 2013-05-04 18:03 ` Paul E. McKenney 2013-04-30 2:52 ` [PATCH v2 2/2] ipvs: Use cond_resched_rcu_lock() helper when dumping connections Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).