From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755784Ab3EASWM (ORCPT ); Wed, 1 May 2013 14:22:12 -0400 Received: from ja.ssi.bg ([178.16.129.10]:37119 "EHLO ja.ssi.bg" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1755617Ab3EASWI (ORCPT ); Wed, 1 May 2013 14:22:08 -0400 Date: Wed, 1 May 2013 21:22:08 +0300 (EEST) From: Julian Anastasov To: Peter Zijlstra cc: Simon Horman , Eric Dumazet , Ingo Molnar , "Paul E. McKenney" , lvs-devel@vger.kernel.org, netdev@vger.kernel.org, netfilter-devel@vger.kernel.org, linux-kernel@vger.kernel.org, Pablo Neira Ayuso , Dipankar Sarma Subject: Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper In-Reply-To: <20130501155501.GB7521@dyad.programming.kicks-ass.net> Message-ID: References: <1367290378-29224-1-git-send-email-horms@verge.net.au> <1367290378-29224-2-git-send-email-horms@verge.net.au> <20130430072944.GA13959@verge.net.au> <20130501091012.GB28253@dyad.programming.kicks-ass.net> <20130501155501.GB7521@dyad.programming.kicks-ass.net> User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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