Re: [PATCH 2/2] ftrace: nmi update statistics

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 30 Oct 2008, Andrew Morton wrote:

> On Thu, 30 Oct 2008 16:08:33 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > This patch adds dynamic ftrace NMI update statistics to the
> > /debugfs/tracing/dyn_ftrace_total_info stat file.
> > 
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > ---
> >  arch/x86/kernel/ftrace.c |   26 ++++++++++++++++++++++++--
> >  kernel/trace/trace.c     |   31 ++++++++++++++++++++++++-------
> 
> No header files were modified, but ftrace_arch_read_dyn_info() should
> have been declared so that the above two compilation units see the
> declaration.

Ah, you caught me being lazy ;-)  Since both also define the function, I 
figured I'd be OK. But you are right, this would prevent bugs where the
two functions somehow got different parameters.

Will fix.

> 
> > 
> > Index: linux-tip.git/arch/x86/kernel/ftrace.c
> > ===================================================================
> > --- linux-tip.git.orig/arch/x86/kernel/ftrace.c	2008-10-30 15:30:12.000000000 -0400
> > +++ linux-tip.git/arch/x86/kernel/ftrace.c	2008-10-30 15:51:51.000000000 -0400
> > @@ -91,6 +91,19 @@ static int mod_code_write;
> >  static void *mod_code_ip;
> >  static void *mod_code_newcode;
> >  
> > +static int nmi_wait_count;
> > +static atomic_t nmi_update_count;
> 
> <the ATOMIC_INIT thing>

Hmm, this being arch specific code, is there such a thing as "atomic 
declared as zero on x86 is OK" rule?

> 
> > +int ftrace_arch_read_dyn_info(char *buf, int size)
> > +{
> > +	int r;
> > +
> > +	r = snprintf(buf, size, "%u %u",
> > +		     nmi_wait_count,
> > +		     atomic_read(&nmi_update_count));
> > +	return r;
> > +}
> 
> Yes, nmi_wait_count is an unsigned quantity.  Might as well define it
> thusly?

OK.

> 
> >  static void ftrace_mod_code(void)
> >  {
> >  	/*
> > @@ -109,8 +122,10 @@ void ftrace_nmi_enter(void)
> >  	atomic_inc(&in_nmi);
> >  	/* Must have in_nmi seen before reading write flag */
> >  	smp_mb();
> > -	if (mod_code_write)
> > +	if (mod_code_write) {
> >  		ftrace_mod_code();
> > +		atomic_inc(&nmi_update_count);
> > +	}
> >  }
> >  
> >  void ftrace_nmi_exit(void)
> > @@ -122,8 +137,15 @@ void ftrace_nmi_exit(void)
> >  
> >  static void wait_for_nmi(void)
> >  {
> > -	while (atomic_read(&in_nmi))
> > +	int waited = 0;
> > +
> > +	while (atomic_read(&in_nmi)) {
> > +		waited = 1;
> >  		cpu_relax();
> > +	}
> > +
> > +	if (waited)
> > +		nmi_wait_count++;
> >  }
> >  
> >  static int
> > Index: linux-tip.git/kernel/trace/trace.c
> > ===================================================================
> > --- linux-tip.git.orig/kernel/trace/trace.c	2008-10-30 15:18:45.000000000 -0400
> > +++ linux-tip.git/kernel/trace/trace.c	2008-10-30 15:51:51.000000000 -0400
> > @@ -2837,22 +2837,39 @@ static struct file_operations tracing_ma
> >  
> >  #ifdef CONFIG_DYNAMIC_FTRACE
> >  
> > +#define DYN_INFO_BUF_SIZE 1023
> > +static char ftrace_dyn_info_buffer[DYN_INFO_BUF_SIZE+1];
> 
> Could be made local to tracing_read_dyn_info().
> 
> 
> Could just be
> 
> 	static char ftrace_dyn_info_buffer[1024];
> 
> then use sizeof/ARRAY_SIZE elsewhere.  I think this is a bit safer and

I think there's a defined macro for that... (goes look)

Yep! it's called ARRAY_SIZE(arr)

> more readable - the reader doesn't have to run around the code checking
> that the correct #define was used in both places.

Will fix.

Thanks,

-- Steve