Re: [PATCH] rudimentary tracing for Classic RCU

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

On Thu, Oct 09, 2008 at 02:20:22PM +0800, Lai Jiangshan wrote:

Thank you for looking this over!

> Paul E. McKenney wrote:

[ . . . ]

> > diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
> > index 9fdba03..ba32338 100644
> > --- a/kernel/Kconfig.preempt
> > +++ b/kernel/Kconfig.preempt
> > @@ -68,7 +68,6 @@ config PREEMPT_RCU
> >  
> >  config RCU_TRACE
> >  	bool "Enable tracing for RCU - currently stats in debugfs"
> > -	depends on PREEMPT_RCU
> >  	select DEBUG_FS
> >  	default y
> >  	help
> 
> I think that we can move it to lib/Kconfig.debug.

Makes sense!

[ . . . ]

> > +static int __init rcuclassic_trace_init(void)
> > +{
> > +	int ret;
> > +
> > +	rcuclassic_trace_buf = kmalloc(RCUCLASSIC_TRACE_BUF_SIZE, GFP_KERNEL);
> 
> I'm wondering about this.
> rcuclassic_trace_buf is kmalloc-ed when init module.
> 
> In this case, why not define it as:
> static char buf[20*NR_CPUS + 100];
> 
> and use it like:
> pos += snprintf(buf + cnt, sizeof(buf) - cnt, ...);

I will make Andi's suggested change, both here and in other previously
submitted code.

[ . . . ]

> > +	if (!rcuclassic_trace_buf)
> > +		return 1;
> > +	ret = rcuclassic_debugfs_init();
> > +	if (ret)
> > +		kfree(rcuclassic_trace_buf);
> > +	return ret;
> > +}
> > +
> > +static void __exit rcuclassic_trace_cleanup(void)
> > +{
> > +	debugfs_remove(cbdir);
> > +	debugfs_remove(rcudir);
> > +	kfree(rcuclassic_trace_buf);
> > +}
> > +
> > +
> > +module_init(rcuclassic_trace_init);
> > +module_exit(rcuclassic_trace_cleanup);
> 
> 
> I think this trace file shows too little vars for classic rcu.
> 
> It is better that this trace file shows more data.
> such as some data in struct rcu_data(quiescent state handling fields, batch, qlen).
> 
> or add new files: rcu/rcu_data<cpu#>

Indeed!  I will add the ability to dump the rcu_data structures, though
I was planning to just dump all of them rather than CPU-by-CPU.  (Same
approach as in rcutree_trace.c patches.)

							Thanx, Paul