LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper

To: Julian Anastasov <ja@xxxxxx>
Subject: Re: [PATCH v2 1/2] sched: Add cond_resched_rcu_lock() helper
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>, Simon Horman <horms@xxxxxxxxxxxx>, Eric Dumazet <eric.dumazet@xxxxxxxxx>, Ingo Molnar <mingo@xxxxxxxxxx>, lvs-devel@xxxxxxxxxxxxxxx, netdev@xxxxxxxxxxxxxxx, netfilter-devel@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>, Dipankar Sarma <dipankar@xxxxxxxxxx>
From: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
Date: Thu, 2 May 2013 12:34:09 -0700
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

--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>