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 [off-list ref] wrote:

quoted

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.

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,

quoted

+
+	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