Re: [PATCH 3/3] tracing: Add 2 new funcs. for kernel access to Ftrace instances.

[Divya Indi]

Hi Steven,

Thanks for taking the time to review. Please find my comments inline.

On 6/8/19 2:51 PM, Steven Rostedt wrote:
> On Tue,  4 Jun 2019 17:42:05 -0700
> Divya Indi <divya.indi@xxxxxxxxxx> wrote:
>
> > Adding 2 new functions -
> > 1) trace_array_lookup : Look up and return a trace array, given its
> > name.
> > 2) trace_array_set_clr_event : Enable/disable event recording to the
> > given trace array.
> >
> > Signed-off-by: Divya Indi <divya.indi@xxxxxxxxxx>
> > ---
> >   include/linux/trace_events.h |  3 +++
> >   kernel/trace/trace.c         | 11 +++++++++++
> >   kernel/trace/trace_events.c  | 22 ++++++++++++++++++++++
> >   3 files changed, 36 insertions(+)
> >
> > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> > index d7b7d85..0cc99a8 100644
> > --- a/include/linux/trace_events.h
> > +++ b/include/linux/trace_events.h
> > @@ -545,7 +545,10 @@ int trace_array_printk(struct trace_array *tr, unsigned long ip,
> >   struct trace_array *trace_array_create(const char *name);
> >   int trace_array_destroy(struct trace_array *tr);
> >   int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
> > +struct trace_array *trace_array_lookup(const char *name);
> >   int trace_set_clr_event(const char *system, const char *event, int set);
> > +int trace_array_set_clr_event(struct trace_array *tr, const char *system,
> > +		const char *event, int set);
> >   
> >   /*
> >    * The double __builtin_constant_p is because gcc will give us an error
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index a60dc13..1d171fd 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -8465,6 +8465,17 @@ static int instance_rmdir(const char *name)
> >   	return ret;
> >   }
> >   
> > +struct trace_array *trace_array_lookup(const char *name)
> > +{
> > +	struct trace_array *tr;
> > +	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> Accessing the ftrace_trace_arrays requires taking the trace_types_lock.
> It should also probably increment the ref counter too, and then
> trace_array_put() needs to be called.
>
> This prevents the trace array from being freed while something has
> access to it.
>
> -- Steve

Agree - Noted!

Also, adding a similar change for trace_array_create which also returns 
a ptr to a newly created trace_array so will face the same issue.

Since trace_array_lookup and trace_array_create will be accompanied by a 
trace_array_destroy once the use of the trace_array is done, 
decrementing the reference ctr here.

Sending a v2 to address this.


Thanks,

Divya

> > +		if (tr->name && strcmp(tr->name, name) == 0)
> > +			return tr;
> > +	}
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(trace_array_lookup);
> > +
> >   static __init void create_trace_instances(struct dentry *d_tracer)
> >   {
> >   	trace_instance_dir = tracefs_create_instance_dir("instances", d_tracer,
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 445b059..c126d2c 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -859,6 +859,28 @@ int trace_set_clr_event(const char *system, const char *event, int set)
> >   }
> >   EXPORT_SYMBOL_GPL(trace_set_clr_event);
> >   
> > +/**
> > + * trace_array_set_clr_event - enable or disable an event for a trace array
> > + * @system: system name to match (NULL for any system)
> > + * @event: event name to match (NULL for all events, within system)
> > + * @set: 1 to enable, 0 to disable
> > + *
> > + * This is a way for other parts of the kernel to enable or disable
> > + * event recording to instances.
> > + *
> > + * Returns 0 on success, -EINVAL if the parameters do not match any
> > + * registered events.
> > + */
> > +int trace_array_set_clr_event(struct trace_array *tr, const char *system,
> > +		const char *event, int set)
> > +{
> > +	if (!tr)
> > +		return -ENODEV;
> > +
> > +	return __ftrace_set_clr_event(tr, NULL, system, event, set);
> > +}
> > +EXPORT_SYMBOL_GPL(trace_array_set_clr_event);
> > +
> >   /* 128 should be much more than enough */
> >   #define EVENT_BUF_SIZE		127
> >