Hi Masami,
On Mon, 2020-01-13 at 22:15 +0900, Masami Hiramatsu wrote:
> Hi Tom,
>
> On Fri, 10 Jan 2020 14:35:08 -0600
> Tom Zanussi <zanussi@xxxxxxxxxx> wrote:
>
> > Add a function to get an event file and prevent it from going away
> > on
> > module or instance removal.
> >
> > get_event_file() will find an event file in a given instance (if
> > instance is NULL, it assumes the top trace array) and return it,
> > pinning the instance's trace array as well as the event's module,
> > if
> > applicable, so they won't go away while in use.
> >
> > put_event_file() does the matching release.
> >
> > Signed-off-by: Tom Zanussi <zanussi@xxxxxxxxxx>
> > ---
> > include/linux/trace_events.h | 5 +++
> > kernel/trace/trace_events.c | 87
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 92 insertions(+)
> >
> > diff --git a/include/linux/trace_events.h
> > b/include/linux/trace_events.h
> > index 4c6e15605766..bc634f4e0158 100644
> > --- a/include/linux/trace_events.h
> > +++ b/include/linux/trace_events.h
> > @@ -348,6 +348,11 @@ enum {
> > EVENT_FILE_FL_WAS_ENABLED_BIT,
> > };
> >
> > +extern struct trace_event_file *get_event_file(const char
> > *instance,
> > + const char *system,
> > + const char *event);
> > +extern void put_event_file(struct trace_event_file *file);
> > +
> > /*
> > * Event file flags:
> > * ENABLED - The event is enabled
> > diff --git a/kernel/trace/trace_events.c
> > b/kernel/trace/trace_events.c
> > index c6de3cebc127..184e10d3772d 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -2535,6 +2535,93 @@ find_event_file(struct trace_array *tr,
> > const char *system, const char *event)
> > return file;
> > }
> >
> > +/**
> > + * get_event_file - Find and return a trace event file
> > + * @instance: The name of the trace instance containing the event
> > + * @system: The name of the system containing the event
> > + * @event: The name of the event
> > + *
> > + * Return a trace event file given the trace instance name, trace
> > + * system, and trace event name. If the instance name is NULL, it
> > + * refers to the top-level trace array.
> > + *
> > + * This function will look it up and return it if found, after
> > calling
> > + * trace_array_get() to prevent the instance from going away, and
> > + * increment the event's module refcount to prevent it from being
> > + * removed.
> > + *
> > + * To release the file, call put_event_file(), which will call
> > + * trace_array_put() and decrement the event's module refcount.
> > + *
> > + * Return: The trace event on success, ERR_PTR otherwise.
> > + */
> > +struct trace_event_file *get_event_file(const char *instance,
> > + const char *system,
> > + const char *event)
> > +{
> > + struct trace_array *tr = top_trace_array();
> > + struct trace_event_file *file = NULL;
> > + int ret = -EINVAL;
> > +
> > + if (instance) {
> > + mutex_lock(&trace_types_lock);
> > + tr = trace_array_find(instance);
> > + mutex_unlock(&trace_types_lock);
> > + if (!tr)
> > + return ERR_PTR(ret);
> > + }
> > +
> > + ret = trace_array_get(tr);
> > + if (ret)
> > + return ERR_PTR(ret);
>
> This seems a bit odd. I think we should introduce
> trace_array_find_get(instance) so that we don't take care of
> loosing tr there. It should be something like
>
> trace_array_find_get(instance) {
> mutex_lock(&trace_types_lock);
> tr = trace_array_find(instance);
> if (tr)
> tr->ref++;
> mutex_unlock(&trace_types_lock);
> return tr;
> }
>
> Then we can write it as below;
>
> struct trace_array *tr = top_trace_array();
>
> if (instance) {
> tr = trace_array_find_get(instance);
> if (!tr)
> return -ENOENT;
> } else
> trace_array_get(tr);/* We never loose top trace_array
> */
> ...
>
> This will fail only if there is no instance.
>
Yes, that's a better idea, will do that in v3.
> BTW, I think the function name should be get_trace_event_file()
> because "event_file" sounds a bit generic. (or trace_get_event_file()
> will give us a good namespace)
>
I agree, will use trace_get_event_file().
Thanks,
Tom
> Thank you,
>
> > +
> > + mutex_lock(&event_mutex);
> > +
> > + file = find_event_file(tr, system, event);
> > + if (!file) {
> > + trace_array_put(tr);
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + /* Don't let event modules unload while in use */
> > + ret = try_module_get(file->event_call->mod);
> > + if (!ret) {
> > + trace_array_put(tr);
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > +
> > + ret = 0;
> > + out:
> > + mutex_unlock(&event_mutex);
> > +
> > + if (ret)
> > + file = ERR_PTR(ret);
> > +
> > + return file;
> > +}
> > +EXPORT_SYMBOL_GPL(get_event_file);
> > +
> > +/**
> > + * put_event_file - Release a file from get_event_file()
> > + * @file: The trace event file
> > + *
> > + * If a file was retrieved using get_event_file(), this should be
> > + * called when it's no longer needed. It will cancel the previous
> > + * trace_array_get() called by that function, and decrement the
> > + * event's module refcount.
> > + */
> > +void put_event_file(struct trace_event_file *file)
> > +{
> > + trace_array_put(file->tr);
> > +
> > + mutex_lock(&event_mutex);
> > + module_put(file->event_call->mod);
> > + mutex_unlock(&event_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(put_event_file);
> > +
> > #ifdef CONFIG_DYNAMIC_FTRACE
> >
> > /* Avoid typos */
> > --
> > 2.14.1
> >
>
>
