Re: [PATCH] netpoll: use non-BH variant of RCU

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Fri, Aug 13, 2010 at 10:39:04AM -0400, Herbert Xu wrote:
> On Thu, Aug 12, 2010 at 08:42:13AM -0700, Paul E. McKenney wrote:
> >
> > +/**
> > + * rcu_read_lock_bh_irqsoff() - mark the beginning of an RCU-bh critical section
> > + *
> > + * This is equivalent of rcu_read_lock_bh(), but to be used where the
> > + * caller either is in an irq handler or has irqs disabled.  Note that
> > + * this function assumes that PREEMPT_RT kernels run irq handlers at
> > + * higher priority than softirq handlers!
> > + */
> > +static inline void rcu_read_lock_bh_irqsoff(void)
> > +{
> > +	rcu_read_unlock_bh_irqsoff_check();
> > +	__acquire(RCU_BH);
> > +	rcu_read_acquire_bh();
> > +}
> 
> Thanks for the patch Paul!
> 
> But this doesn't really solve the problem for netif_rx.  The reason
> is that netif_rx can either be called with IRQs on OR off.  So we
> need to take the right precautions in the case where IRQs are
> enabled along with BH.

Interesting...

Is it possible that IRQs are off at rcu_read_lock_bh_irqsoff() time, but
enabled by the time we get to rcu_read_unlock_bh_irqsoff()?  I hope not,
but have to ask.  If I am guaranteed of the same state in both cases,
I can do something like the following:

static inline void rcu_read_lock_bh_irqsoff(void)
{
	if (!in_irq() && !irqs_disabled())
		local_bh_disable();
	__acquire(RCU_BH);
	rcu_read_acquire_bh();
}

static inline void rcu_read_unlock_bh_irqsoff(void)
{
	rcu_read_release_bh();
	__release(RCU_BH);
	if (!in_irq() && !irqs_disabled())
		local_bh_enable();
}

If the state can change in the RCU-bh read-side critical section, then
I would have to record the state in the task structure or some such.

But all in all, mightn't it be easier to remove the checks from
_local_bh_enable(), and then just use rcu_read_lock_bh()?  Have those
checks really been that helpful in finding bugs?  ;-)

							Thanx, Paul