Re: [PATCH 3/3] rcu: Call trace_rcu_batch_start() with enabled interrupts

[Alexander Gordeev <agordeev@redhat.com>]

On Wed, Feb 11, 2015 at 08:13:30AM -0800, Paul E. McKenney wrote:
> On Wed, Feb 11, 2015 at 03:42:39PM +0100, Alexander Gordeev wrote:
> > Currently trace_rcu_batch_start() is called with local
> > interrupts disabled. Yet, there is no reason to do so.
> > 
> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> 
> Hmmm...  I am not seeing this one.  As you noted in the commit log for
> your earlier patch, the purpose of Tiny RCU is to be tiny, not to be
> all that fast.  This commit increases the size a bit (admittedly only
> when CONFIG_RCU_TRACE=y), and also increases complexity a bit.
> 
> So it does not look to me to be something we want for Tiny RCU.
> 
> So what am I missing here?

The benefit - "heavy" trace_rcu_batch_start() is called while interrupts
are enabled. Which is normally a priority, but in this case - still a
good tradeoff IMHO.

And I do not agree :) The code reads better with the loop tightly "enclosed"
with trace_rcu_batch_start()/trace_rcu_batch_end().

> 							Thanx, Paul
> 
> > ---
> >  kernel/rcu/tiny.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
> > index 069742d..01e80ac 100644
> > --- a/kernel/rcu/tiny.c
> > +++ b/kernel/rcu/tiny.c
> > @@ -166,11 +166,12 @@ static void __rcu_process_callbacks(struct rcu_ctrlblk *rcp)
> >  	const char *rn = NULL;
> >  	struct rcu_head *next, *list;
> >  	unsigned long flags;
> > +	RCU_TRACE(long qlen);
> >  	RCU_TRACE(int cb_count = 0);
> > 
> >  	/* Move the ready-to-invoke callbacks to a local list. */
> >  	local_irq_save(flags);
> > -	RCU_TRACE(trace_rcu_batch_start(rcp->name, 0, rcp->qlen, -1));
> > +	RCU_TRACE(qlen = rcp->qlen);
> >  	list = rcp->rcucblist;
> >  	rcp->rcucblist = *rcp->donetail;
> >  	*rcp->donetail = NULL;
> > @@ -180,6 +181,7 @@ static void __rcu_process_callbacks(struct rcu_ctrlblk *rcp)
> >  	local_irq_restore(flags);
> > 
> >  	/* Invoke the callbacks on the local list. */
> > +	RCU_TRACE(trace_rcu_batch_start(rcp->name, 0, qlen, -1));
> >  	RCU_TRACE(rn = rcp->name);
> >  	while (list) {
> >  		next = list->next;
> > -- 
> > 1.8.3.1
> > 
> 

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com