Re: [PATCH v2 02/12] tracing: Add get/put_event_file()

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Tom,

On Fri, 10 Jan 2020 14:35:08 -0600
Tom Zanussi <zanussi@kernel.org> 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@kernel.org>
> ---
>  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.

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)

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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>