Re: [PATCH] samples: ftrace: make some global variables static

[Tom Rix]

On 1/31/23 2:09 AM, Mark Rutland wrote:
> On Mon, Jan 30, 2023 at 11:37:08AM -0800, Tom Rix wrote:
> > smatch reports this representative issue
> > samples/ftrace/ftrace-ops.c:15:14: warning: symbol 'nr_function_calls' was not declared. Should it be static?
> >
> > The nr_functions_calls and several other global variables are only
> > used in ftrace-ops.c, so they should be static.
> This makes sense to me.
>
> > Remove the instances of initializing static int to 0.

Checkpatch complains loudly about the initializations, so I removed them.

bool is an int type and one of the things checkpatch caught.

Tom

> I appreciate that static variables get implicitly zero initialized, but
> dropping the initialization is inconsistent with the other control variables,
> and IMO it's quite confusing, so I'd prefer to keep that for clarity. I note
> you've also dropped the initialization of a bool to false, whereas the above
> just mentions int.
>
> I'd prefer to keep the initialization, but I'll defre to Steve if he thinks
> differently. :)
>
> > Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
> With the above taken into account:
>
> Acked-by: Mark Rutland <mark.rutland@xxxxxxx>
>
> Thanks,
> Mark.
>
> > ---
> >   samples/ftrace/ftrace-ops.c | 24 ++++++++++++------------
> >   1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/samples/ftrace/ftrace-ops.c b/samples/ftrace/ftrace-ops.c
> > index 24deb51c7261..5e891a995dd3 100644
> > --- a/samples/ftrace/ftrace-ops.c
> > +++ b/samples/ftrace/ftrace-ops.c
> > @@ -12,7 +12,7 @@
> >    * Arbitrary large value chosen to be sufficiently large to minimize noise but
> >    * sufficiently small to complete quickly.
> >    */
> > -unsigned int nr_function_calls = 100000;
> > +static unsigned int nr_function_calls = 100000;
> >   module_param(nr_function_calls, uint, 0);
> >   MODULE_PARM_DESC(nr_function_calls, "How many times to call the relevant tracee");
> >   
> > @@ -21,7 +21,7 @@ MODULE_PARM_DESC(nr_function_calls, "How many times to call the relevant tracee"
> >    * be called directly or whether it's necessary to go via the list func, which
> >    * can be significantly more expensive.
> >    */
> > -unsigned int nr_ops_relevant = 1;
> > +static unsigned int nr_ops_relevant = 1;
> >   module_param(nr_ops_relevant, uint, 0);
> >   MODULE_PARM_DESC(nr_ops_relevant, "How many ftrace_ops to associate with the relevant tracee");
> >   
> > @@ -30,7 +30,7 @@ MODULE_PARM_DESC(nr_ops_relevant, "How many ftrace_ops to associate with the rel
> >    * tracers enabled for distinct functions can force the use of the list func
> >    * and incur overhead for all call sites.
> >    */
> > -unsigned int nr_ops_irrelevant = 0;
> > +static unsigned int nr_ops_irrelevant;
> >   module_param(nr_ops_irrelevant, uint, 0);
> >   MODULE_PARM_DESC(nr_ops_irrelevant, "How many ftrace_ops to associate with the irrelevant tracee");
> >   
> > @@ -38,15 +38,15 @@ MODULE_PARM_DESC(nr_ops_irrelevant, "How many ftrace_ops to associate with the i
> >    * On architectures with DYNAMIC_FTRACE_WITH_REGS, saving the full pt_regs can
> >    * be more expensive than only saving the minimal necessary regs.
> >    */
> > -bool save_regs = false;
> > +static bool save_regs;
> >   module_param(save_regs, bool, 0);
> >   MODULE_PARM_DESC(save_regs, "Register ops with FTRACE_OPS_FL_SAVE_REGS (save all registers in the trampoline)");
> >   
> > -bool assist_recursion = false;
> > +static bool assist_recursion;
> >   module_param(assist_recursion, bool, 0);
> >   MODULE_PARM_DESC(assist_reursion, "Register ops with FTRACE_OPS_FL_RECURSION");
> >   
> > -bool assist_rcu = false;
> > +static bool assist_rcu;
> >   module_param(assist_rcu, bool, 0);
> >   MODULE_PARM_DESC(assist_reursion, "Register ops with FTRACE_OPS_FL_RCU");
> >   
> > @@ -55,7 +55,7 @@ MODULE_PARM_DESC(assist_reursion, "Register ops with FTRACE_OPS_FL_RCU");
> >    * overhead. Sometimes a consistency check using a more expensive tracer is
> >    * desireable.
> >    */
> > -bool check_count = false;
> > +static bool check_count;
> >   module_param(check_count, bool, 0);
> >   MODULE_PARM_DESC(check_count, "Check that tracers are called the expected number of times\n");
> >   
> > @@ -64,7 +64,7 @@ MODULE_PARM_DESC(check_count, "Check that tracers are called the expected number
> >    * runs, but sometimes it can be useful to leave them registered so that they
> >    * can be inspected through the tracefs 'enabled_functions' file.
> >    */
> > -bool persist = false;
> > +static bool persist;
> >   module_param(persist, bool, 0);
> >   MODULE_PARM_DESC(persist, "Successfully load module and leave ftrace ops registered after test completes\n");
> >   
> > @@ -114,8 +114,8 @@ static void ops_func_count(unsigned long ip, unsigned long parent_ip,
> >   	self->count++;
> >   }
> >   
> > -struct sample_ops *ops_relevant;
> > -struct sample_ops *ops_irrelevant;
> > +static struct sample_ops *ops_relevant;
> > +static struct sample_ops *ops_irrelevant;
> >   
> >   static struct sample_ops *ops_alloc_init(void *tracee, ftrace_func_t func,
> >   					 unsigned long flags, int nr)
> > @@ -163,8 +163,8 @@ static void ops_check(struct sample_ops *ops, int nr,
> >   	}
> >   }
> >   
> > -ftrace_func_t tracer_relevant = ops_func_nop;
> > -ftrace_func_t tracer_irrelevant = ops_func_nop;
> > +static ftrace_func_t tracer_relevant = ops_func_nop;
> > +static ftrace_func_t tracer_irrelevant = ops_func_nop;
> >   
> >   static int __init ftrace_ops_sample_init(void)
> >   {
> > --
> > 2.26.3