| Message ID | 20130501155501.GB7521@dyad.programming.kicks-ass.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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> -- 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
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> > -- 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
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.. -- 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
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> -- 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
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. *