[PATCH 0/5 v4]Kernel Access to Ftrace instances.

[PATCH 0/5 v4]Kernel Access to Ftrace instances.

[Divya Indi]

In addition to patches introduced by commit f45d1225adb0 "tracing: Kernel
access to Ftrace instances") we also need the following patches to reliably
access ftrace instances from other kernel modules or components.

This version addresses the review comments/suggestions received for v3.

Please review the patches that follow.

Divya Indi (5):
  tracing: Declare newly exported APIs in include/linux/trace.h
  tracing: Verify if trace array exists before destroying it.
  tracing: Adding NULL checks
  tracing: Handle the trace array ref counter in new functions
  tracing: New functions for kernel access to Ftrace instances

 include/linux/trace.h        | 10 +++++
 include/linux/trace_events.h |  3 +-
 kernel/trace/trace.c         | 88 ++++++++++++++++++++++++++++++++++++++++++--
 kernel/trace/trace.h         |  4 +-
 kernel/trace/trace_events.c  | 25 ++++++++++++-
 5 files changed, 121 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] tracing: Declare newly exported APIs in include/linux/trace.h

[Divya Indi]

Declare the newly introduced and exported APIs in the header file -
include/linux/trace.h. Moving previous declarations from
kernel/trace/trace.h to include/linux/trace.h.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
---
 include/linux/trace.h | 7 +++++++
 kernel/trace/trace.h  | 4 +---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/trace.h b/include/linux/trace.h
index b95ffb2..24fcf07 100644
--- a/include/linux/trace.h
+++ b/include/linux/trace.h
@@ -24,6 +24,13 @@ struct trace_export {
 int register_ftrace_export(struct trace_export *export);
 int unregister_ftrace_export(struct trace_export *export);
 
+struct trace_array;
+
+void trace_printk_init_buffers(void);
+int trace_array_printk(struct trace_array *tr, unsigned long ip,
+		const char *fmt, ...);
+struct trace_array *trace_array_create(const char *name);
+int trace_array_destroy(struct trace_array *tr);
 #endif	/* CONFIG_TRACING */
 
 #endif	/* _LINUX_TRACE_H */
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 005f086..66ff63e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -11,6 +11,7 @@
 #include <linux/mmiotrace.h>
 #include <linux/tracepoint.h>
 #include <linux/ftrace.h>
+#include <linux/trace.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/trace_seq.h>
 #include <linux/trace_events.h>
@@ -852,8 +853,6 @@ extern int trace_selftest_startup_branch(struct tracer *trace,
 extern int
 trace_array_vprintk(struct trace_array *tr,
 		    unsigned long ip, const char *fmt, va_list args);
-int trace_array_printk(struct trace_array *tr,
-		       unsigned long ip, const char *fmt, ...);
 int trace_array_printk_buf(struct ring_buffer *buffer,
 			   unsigned long ip, const char *fmt, ...);
 void trace_printk_seq(struct trace_seq *s);
@@ -1869,7 +1868,6 @@ extern int trace_event_enable_disable(struct trace_event_file *file,
 extern const char *__stop___tracepoint_str[];
 
 void trace_printk_control(bool enabled);
-void trace_printk_init_buffers(void);
 void trace_printk_start_comm(void);
 int trace_keep_overwrite(struct tracer *tracer, u32 mask, int set);
 int set_tracer_flag(struct trace_array *tr, unsigned int mask, int enabled);
-- 
1.8.3.1

[PATCH 2/5] tracing: Verify if trace array exists before destroying it.

[Divya Indi]

A trace array can be destroyed from userspace or kernel. Verify if the
trace exists before proceeding to destroy/remove it.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
---
 kernel/trace/trace.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1c80521..468ecc5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8421,17 +8421,27 @@ static int __remove_instance(struct trace_array *tr)
 	return 0;
 }
 
-int trace_array_destroy(struct trace_array *tr)
+int trace_array_destroy(struct trace_array *this_tr)
 {
+	struct trace_array *tr;
 	int ret;
 
-	if (!tr)
+	if (!this_tr) {
 		return -EINVAL;
+	}
 
 	mutex_lock(&event_mutex);
 	mutex_lock(&trace_types_lock);
 
-	ret = __remove_instance(tr);
+	ret = -ENODEV;
+
+	/* Making sure trace array exists before destroying it. */
+	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+		if (tr == this_tr) {
+			ret = __remove_instance(tr);
+			break;
+		}
+	}
 
 	mutex_unlock(&trace_types_lock);
 	mutex_unlock(&event_mutex);
-- 
1.8.3.1

[PATCH 3/5] tracing: Adding NULL checks

[Divya Indi]

As part of commit f45d1225adb0 ("tracing: Kernel access to Ftrace
instances") we exported certain functions. Here, we are adding some additional
NULL checks to ensure safe usage by users of these APIs.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
---
 kernel/trace/trace.c        | 3 +++
 kernel/trace/trace_events.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 468ecc5..22bf166 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3205,6 +3205,9 @@ int trace_array_printk(struct trace_array *tr,
 	if (!(global_trace.trace_flags & TRACE_ITER_PRINTK))
 		return 0;
 
+	if (!tr)
+		return -ENOENT;
+
 	va_start(ap, fmt);
 	ret = trace_array_vprintk(tr, ip, fmt, ap);
 	va_end(ap);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 0ce3db6..2621995 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -800,6 +800,8 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
 	char *event = NULL, *sub = NULL, *match;
 	int ret;
 
+	if (!tr)
+		return -ENOENT;
 	/*
 	 * The buf format can be <subsystem>:<event-name>
 	 *  *:<event-name> means any event by that name.
-- 
1.8.3.1

[PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Divya Indi]

For functions returning a trace array Eg: trace_array_create(), we need to
increment the reference counter associated with the trace array to ensure it
does not get freed when in use.

Once we are done using the trace array, we need to call
trace_array_put() to make sure we are not holding a reference to it
anymore and the instance/trace array can be removed when required.

Hence, additionally exporting trace_array_put().

Example use:

tr = trace_array_create("foo-bar");
// Use this trace array
// Log to this trace array or enable/disable events to this trace array.
....
....
// tr no longer required
trace_array_put();

Signed-off-by: Divya Indi <divya.indi@oracle.com>
---
 include/linux/trace.h |  1 +
 kernel/trace/trace.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/include/linux/trace.h b/include/linux/trace.h
index 24fcf07..2c782d5 100644
--- a/include/linux/trace.h
+++ b/include/linux/trace.h
@@ -31,6 +31,7 @@ int trace_array_printk(struct trace_array *tr, unsigned long ip,
 		const char *fmt, ...);
 struct trace_array *trace_array_create(const char *name);
 int trace_array_destroy(struct trace_array *tr);
+void trace_array_put(struct trace_array *tr);
 #endif	/* CONFIG_TRACING */
 
 #endif	/* _LINUX_TRACE_H */
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 22bf166..7b6a37a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -297,12 +297,22 @@ static void __trace_array_put(struct trace_array *this_tr)
 	this_tr->ref--;
 }
 
+/**
+ * trace_array_put - Decrement reference counter for the given trace array.
+ * @tr: Trace array for which reference counter needs to decrement.
+ *
+ * NOTE: Functions like trace_array_create increment the reference counter for
+ * the trace array to ensure they do not get freed while in use. Make sure to
+ * call trace_array_put() when the use is done. This will ensure that the
+ * instance can be later removed.
+ */
 void trace_array_put(struct trace_array *this_tr)
 {
 	mutex_lock(&trace_types_lock);
 	__trace_array_put(this_tr);
 	mutex_unlock(&trace_types_lock);
 }
+EXPORT_SYMBOL_GPL(trace_array_put);
 
 int call_filter_check_discard(struct trace_event_call *call, void *rec,
 			      struct ring_buffer *buffer,
@@ -8302,6 +8312,16 @@ static void update_tracer_options(struct trace_array *tr)
 	mutex_unlock(&trace_types_lock);
 }
 
+/**
+ * trace_array_create - Create a new trace array with the given name.
+ * @name: The name of the trace array to be created.
+ *
+ * Create and return a trace array with given name if it does not exist.
+ *
+ * NOTE: The reference counter associated with the returned trace array is
+ * incremented to ensure it is not freed when in use. Make sure to call
+ * "trace_array_put" for this trace array when its use is done.
+ */
 struct trace_array *trace_array_create(const char *name)
 {
 	struct trace_array *tr;
@@ -8364,6 +8384,8 @@ struct trace_array *trace_array_create(const char *name)
 
 	list_add(&tr->list, &ftrace_trace_arrays);
 
+	tr->ref++;
+
 	mutex_unlock(&trace_types_lock);
 	mutex_unlock(&event_mutex);
 
@@ -8385,7 +8407,19 @@ struct trace_array *trace_array_create(const char *name)
 
 static int instance_mkdir(const char *name)
 {
-	return PTR_ERR_OR_ZERO(trace_array_create(name));
+	struct trace_array *tr;
+
+	tr = trace_array_create(name);
+	if (IS_ERR(tr))
+		return PTR_ERR(tr);
+
+	/* This function does not return a reference to the created trace array,
+	 * so the reference counter is to be 0 when it returns.
+	 * trace_array_create increments the ref counter, decrement it here
+	 * by calling trace_array_put() */
+	trace_array_put(tr);
+
+	return 0;
 }
 
 static int __remove_instance(struct trace_array *tr)
@@ -8424,6 +8458,11 @@ static int __remove_instance(struct trace_array *tr)
 	return 0;
 }
 
+/*
+ * NOTE: An instance cannot be removed if there is still a reference to it.
+ * Make sure to call "trace_array_put" for a trace array returned by functions
+ * like trace_array_create(), otherwise trace_array_destroy will not succeed.
+ */
 int trace_array_destroy(struct trace_array *this_tr)
 {
 	struct trace_array *tr;
-- 
1.8.3.1

[PATCH 5/5] tracing: New functions for kernel access to Ftrace instances

[Divya Indi]

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.

NOTE: trace_array_lookup returns a trace array and also increments the
reference counter associated with the returned trace array. Make sure to
call trace_array_put() once the use is done so that the instance can be
removed at a later time.

Example use:

tr = trace_array_lookup("foo-bar");
if (!tr)
	tr = trace_array_create("foo-bar");
// Log to tr
// Enable/disable events to tr
trace_array_set_clr_event(tr, _THIS_IP,"system","event",1);

// Done using tr
trace_array_put(tr);
..

Also, use trace_array_set_clr_event to enable/disable events to a trace array.
So now we no longer need to have ftrace_set_clr_event as an exported
API.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
---
 include/linux/trace.h        |  2 ++
 include/linux/trace_events.h |  3 ++-
 kernel/trace/trace.c         | 28 ++++++++++++++++++++++++++++
 kernel/trace/trace_events.c  | 23 ++++++++++++++++++++++-
 4 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/include/linux/trace.h b/include/linux/trace.h
index 2c782d5..05164bb 100644
--- a/include/linux/trace.h
+++ b/include/linux/trace.h
@@ -32,6 +32,8 @@ 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);
 void trace_array_put(struct trace_array *tr);
+struct trace_array *trace_array_lookup(const char *name);
+
 #endif	/* CONFIG_TRACING */
 
 #endif	/* _LINUX_TRACE_H */
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 8a62731..05a7514 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -540,7 +540,8 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
 #define is_signed_type(type)	(((type)(-1)) < (type)1)
 
 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
  * if we try to allocate the static variable to fmt if it is not a
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 7b6a37a..e394d55 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8514,6 +8514,34 @@ static int instance_rmdir(const char *name)
 	return ret;
 }
 
+/**
+ * trace_array_lookup - Lookup the trace array, given its name.
+ * @name: The name of the trace array to be looked up.
+ *
+ * Lookup and return the trace array associated with @name.
+ *
+ * NOTE: The reference counter associated with the returned trace array is
+ * incremented to ensure it is not freed when in use. Make sure to call
+ * "trace_array_put" for this trace array when its use is done.
+ */
+struct trace_array *trace_array_lookup(const char *name)
+{
+	struct trace_array *tr;
+
+	mutex_lock(&trace_types_lock);
+
+	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+		if (tr->name && strcmp(tr->name, name) == 0) {
+			tr->ref++;
+			mutex_unlock(&trace_types_lock);
+			return tr;
+		}
+	}
+	mutex_unlock(&trace_types_lock);
+	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 2621995..96dd997 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -834,7 +834,6 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
 
 /**
  * trace_set_clr_event - enable or disable an event
@@ -859,6 +858,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 -ENOENT;
+
+	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
 
-- 
1.8.3.1

Re: [PATCH 2/5] tracing: Verify if trace array exists before destroying it.

[Aruna Ramakrishna]

On 08/14/2019 10:55 AM, Divya Indi wrote:
> A trace array can be destroyed from userspace or kernel. Verify if the
> trace exists before proceeding to destroy/remove it.
>

^^ s/trace/trace array/

As you pointed out yourself. :)

All the patches look good to me. For this patchset:

Reviewed-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>

Thanks,
Aruna

Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Steven Rostedt]

Sorry for taking so long to getting to these patches.

On Wed, 14 Aug 2019 10:55:26 -0700
Divya Indi <divya.indi@oracle.com> wrote:

> For functions returning a trace array Eg: trace_array_create(), we need to
> increment the reference counter associated with the trace array to ensure it
> does not get freed when in use.
> 
> Once we are done using the trace array, we need to call
> trace_array_put() to make sure we are not holding a reference to it
> anymore and the instance/trace array can be removed when required.

I think it would be more in line with other parts of the kernel if we
don't need to do the trace_array_put() before calling
trace_array_destroy().

That is, if we make the following change:

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 5ff206ce259e..ae12aac21c6c 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8527,9 +8527,11 @@ static int __remove_instance(struct trace_array *tr)
 {
 	int i;
 
-	if (tr->ref || (tr->current_trace && tr->current_trace->ref))
+	if (tr->ref > 1 || (tr->current_trace && tr->current_trace->ref))
 		return -EBUSY;
 
+	WARN_ON_ONCE(tr->ref != 1);
+
 	list_del(&tr->list);
 
 	/* Disable all the flags that were enabled coming in */

> 
> Hence, additionally exporting trace_array_put().
> 
> Example use:
> 
> tr = trace_array_create("foo-bar");
> // Use this trace array
> // Log to this trace array or enable/disable events to this trace array.
> ....
> ....
> // tr no longer required
> trace_array_put();
> 
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> ---
>  include/linux/trace.h |  1 +
>  kernel/trace/trace.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/trace.h b/include/linux/trace.h
> index 24fcf07..2c782d5 100644
> --- a/include/linux/trace.h
> +++ b/include/linux/trace.h
> @@ -31,6 +31,7 @@ int trace_array_printk(struct trace_array *tr, unsigned long ip,
>  		const char *fmt, ...);
>  struct trace_array *trace_array_create(const char *name);
>  int trace_array_destroy(struct trace_array *tr);
> +void trace_array_put(struct trace_array *tr);
>  #endif	/* CONFIG_TRACING */
>  
>  #endif	/* _LINUX_TRACE_H */
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 22bf166..7b6a37a 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -297,12 +297,22 @@ static void __trace_array_put(struct trace_array *this_tr)
>  	this_tr->ref--;
>  }
>  
> +/**
> + * trace_array_put - Decrement reference counter for the given trace array.
> + * @tr: Trace array for which reference counter needs to decrement.
> + *
> + * NOTE: Functions like trace_array_create increment the reference counter for
> + * the trace array to ensure they do not get freed while in use. Make sure to
> + * call trace_array_put() when the use is done. This will ensure that the
> + * instance can be later removed.
> + */
>  void trace_array_put(struct trace_array *this_tr)
>  {
>  	mutex_lock(&trace_types_lock);
>  	__trace_array_put(this_tr);
>  	mutex_unlock(&trace_types_lock);
>  }
> +EXPORT_SYMBOL_GPL(trace_array_put);
>  
>  int call_filter_check_discard(struct trace_event_call *call, void *rec,
>  			      struct ring_buffer *buffer,
> @@ -8302,6 +8312,16 @@ static void update_tracer_options(struct trace_array *tr)
>  	mutex_unlock(&trace_types_lock);
>  }
>  
> +/**
> + * trace_array_create - Create a new trace array with the given name.
> + * @name: The name of the trace array to be created.
> + *
> + * Create and return a trace array with given name if it does not exist.
> + *
> + * NOTE: The reference counter associated with the returned trace array is
> + * incremented to ensure it is not freed when in use. Make sure to call
> + * "trace_array_put" for this trace array when its use is done.
> + */
>  struct trace_array *trace_array_create(const char *name)
>  {
>  	struct trace_array *tr;
> @@ -8364,6 +8384,8 @@ struct trace_array *trace_array_create(const char *name)
>  
>  	list_add(&tr->list, &ftrace_trace_arrays);
>  
> +	tr->ref++;
> +
>  	mutex_unlock(&trace_types_lock);
>  	mutex_unlock(&event_mutex);
>  
> @@ -8385,7 +8407,19 @@ struct trace_array *trace_array_create(const char *name)
>  
>  static int instance_mkdir(const char *name)
>  {
> -	return PTR_ERR_OR_ZERO(trace_array_create(name));
> +	struct trace_array *tr;
> +
> +	tr = trace_array_create(name);
> +	if (IS_ERR(tr))
> +		return PTR_ERR(tr);
> +
> +	/* This function does not return a reference to the created trace array,
> +	 * so the reference counter is to be 0 when it returns.
> +	 * trace_array_create increments the ref counter, decrement it here
> +	 * by calling trace_array_put() */
> +	trace_array_put(tr);
> +

If we make it that the destroy needs tr->ref == 1, we can remove the
above.

> +	return 0;
>  }
>  
>  static int __remove_instance(struct trace_array *tr)
> @@ -8424,6 +8458,11 @@ static int __remove_instance(struct trace_array *tr)
>  	return 0;
>  }
>  
> +/*
> + * NOTE: An instance cannot be removed if there is still a reference to it.
> + * Make sure to call "trace_array_put" for a trace array returned by functions
> + * like trace_array_create(), otherwise trace_array_destroy will not succeed.
> + */

And remove the above comment too.

-- Steve

>  int trace_array_destroy(struct trace_array *this_tr)
>  {
>  	struct trace_array *tr;

Re: [PATCH 5/5] tracing: New functions for kernel access to Ftrace instances

[Steven Rostedt]

On Wed, 14 Aug 2019 10:55:27 -0700
Divya Indi <divya.indi@oracle.com> 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.
> 
> NOTE: trace_array_lookup returns a trace array and also increments the
> reference counter associated with the returned trace array. Make sure to
> call trace_array_put() once the use is done so that the instance can be
> removed at a later time.
> 
> Example use:
> 
> tr = trace_array_lookup("foo-bar");

Should probably be renamed to: trace_array_get_by_name("foo-bar")

As the name should show that it adds a ref count.

But if we make the change I suggested before, where we do not need to
do the put before calling the destroy, we should have:


> if (!tr)
{
> 	tr = trace_array_create("foo-bar");
	trace_array_get(tr);
}


> // Log to tr
> // Enable/disable events to tr
> trace_array_set_clr_event(tr, _THIS_IP,"system","event",1);

What's with the __THIS_IP?


> 
> // Done using tr
> trace_array_put(tr);
> ..
> 
> Also, use trace_array_set_clr_event to enable/disable events to a trace array.
> So now we no longer need to have ftrace_set_clr_event as an exported
> API.
> 
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> ---
>  include/linux/trace.h        |  2 ++
>  include/linux/trace_events.h |  3 ++-
>  kernel/trace/trace.c         | 28 ++++++++++++++++++++++++++++
>  kernel/trace/trace_events.c  | 23 ++++++++++++++++++++++-
>  4 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/trace.h b/include/linux/trace.h
> index 2c782d5..05164bb 100644
> --- a/include/linux/trace.h
> +++ b/include/linux/trace.h
> @@ -32,6 +32,8 @@ 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);
>  void trace_array_put(struct trace_array *tr);
> +struct trace_array *trace_array_lookup(const char *name);
> +
>  #endif	/* CONFIG_TRACING */
>  
>  #endif	/* _LINUX_TRACE_H */
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index 8a62731..05a7514 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -540,7 +540,8 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
>  #define is_signed_type(type)	(((type)(-1)) < (type)1)
>  
>  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
>   * if we try to allocate the static variable to fmt if it is not a
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 7b6a37a..e394d55 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -8514,6 +8514,34 @@ static int instance_rmdir(const char *name)
>  	return ret;
>  }
>  
> +/**
> + * trace_array_lookup - Lookup the trace array, given its name.
> + * @name: The name of the trace array to be looked up.
> + *
> + * Lookup and return the trace array associated with @name.
> + *
> + * NOTE: The reference counter associated with the returned trace array is
> + * incremented to ensure it is not freed when in use. Make sure to call
> + * "trace_array_put" for this trace array when its use is done.
> + */
> +struct trace_array *trace_array_lookup(const char *name)
> +{
> +	struct trace_array *tr;
> +
> +	mutex_lock(&trace_types_lock);
> +
> +	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> +		if (tr->name && strcmp(tr->name, name) == 0) {
> +			tr->ref++;
> +			mutex_unlock(&trace_types_lock);
> +			return tr;
> +		}
> +	}
> +	mutex_unlock(&trace_types_lock);
> +	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 2621995..96dd997 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -834,7 +834,6 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
>  
>  /**
>   * trace_set_clr_event - enable or disable an event
> @@ -859,6 +858,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

We probably should make this a boolean.

-- Steve

> + *
> + * 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 -ENOENT;
> +
> +	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
>  

Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Divya Indi]

Hi Steve,

Thanks again for taking the time to review and providing feedback. Please find my comments inline.

On 10/15/19 4:04 PM, Steven Rostedt wrote:
> Sorry for taking so long to getting to these patches.
>
> On Wed, 14 Aug 2019 10:55:26 -0700
> Divya Indi <divya.indi@oracle.com> wrote:
>
>> For functions returning a trace array Eg: trace_array_create(), we need to
>> increment the reference counter associated with the trace array to ensure it
>> does not get freed when in use.
>>
>> Once we are done using the trace array, we need to call
>> trace_array_put() to make sure we are not holding a reference to it
>> anymore and the instance/trace array can be removed when required.
> I think it would be more in line with other parts of the kernel if we
> don't need to do the trace_array_put() before calling
> trace_array_destroy().

The reason we went with this approach is

instance_mkdir -          ref_ctr = 0  // Does not return a trace array ptr.
trace_array_create -      ref_ctr = 1  // Since this returns a trace array ptr.
trace_array_lookup -      ref_ctr = 1  // Since this returns a trace array ptr.

if we make trace_array_destroy to expect ref_ctr to be 1, we risk destroying the trace array while in use.

We could make it -

instance_mkdir - 	ref_ctr = 1
trace_array_create -    ref_ctr = 2
trace_array_lookup -    ref_ctr = 2+  // depending on no of lookups

but, we'd still need the trace_array_put() (?)

We can also have one function doing create (if does not exist) or lookup (if exists), but that would require
some redundant code since instance_mkdir needs to return -EXIST when a trace array already exists.

Let me know your thoughts on this.

Thanks,
Divya

> That is, if we make the following change:
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 5ff206ce259e..ae12aac21c6c 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -8527,9 +8527,11 @@ static int __remove_instance(struct trace_array *tr)
>   {
>   	int i;
>   
> -	if (tr->ref || (tr->current_trace && tr->current_trace->ref))
> +	if (tr->ref > 1 || (tr->current_trace && tr->current_trace->ref))
>   		return -EBUSY;
>   
> +	WARN_ON_ONCE(tr->ref != 1);
> +
>   	list_del(&tr->list);
>   
>   	/* Disable all the flags that were enabled coming in */
>
>> Hence, additionally exporting trace_array_put().
>>
>> Example use:
>>
>> tr = trace_array_create("foo-bar");
>> // Use this trace array
>> // Log to this trace array or enable/disable events to this trace array.
>> ....
>> ....
>> // tr no longer required
>> trace_array_put();
>>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> ---
>>   include/linux/trace.h |  1 +
>>   kernel/trace/trace.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/trace.h b/include/linux/trace.h
>> index 24fcf07..2c782d5 100644
>> --- a/include/linux/trace.h
>> +++ b/include/linux/trace.h
>> @@ -31,6 +31,7 @@ int trace_array_printk(struct trace_array *tr, unsigned long ip,
>>   		const char *fmt, ...);
>>   struct trace_array *trace_array_create(const char *name);
>>   int trace_array_destroy(struct trace_array *tr);
>> +void trace_array_put(struct trace_array *tr);
>>   #endif	/* CONFIG_TRACING */
>>   
>>   #endif	/* _LINUX_TRACE_H */
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 22bf166..7b6a37a 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -297,12 +297,22 @@ static void __trace_array_put(struct trace_array *this_tr)
>>   	this_tr->ref--;
>>   }
>>   
>> +/**
>> + * trace_array_put - Decrement reference counter for the given trace array.
>> + * @tr: Trace array for which reference counter needs to decrement.
>> + *
>> + * NOTE: Functions like trace_array_create increment the reference counter for
>> + * the trace array to ensure they do not get freed while in use. Make sure to
>> + * call trace_array_put() when the use is done. This will ensure that the
>> + * instance can be later removed.
>> + */
>>   void trace_array_put(struct trace_array *this_tr)
>>   {
>>   	mutex_lock(&trace_types_lock);
>>   	__trace_array_put(this_tr);
>>   	mutex_unlock(&trace_types_lock);
>>   }
>> +EXPORT_SYMBOL_GPL(trace_array_put);
>>   
>>   int call_filter_check_discard(struct trace_event_call *call, void *rec,
>>   			      struct ring_buffer *buffer,
>> @@ -8302,6 +8312,16 @@ static void update_tracer_options(struct trace_array *tr)
>>   	mutex_unlock(&trace_types_lock);
>>   }
>>   
>> +/**
>> + * trace_array_create - Create a new trace array with the given name.
>> + * @name: The name of the trace array to be created.
>> + *
>> + * Create and return a trace array with given name if it does not exist.
>> + *
>> + * NOTE: The reference counter associated with the returned trace array is
>> + * incremented to ensure it is not freed when in use. Make sure to call
>> + * "trace_array_put" for this trace array when its use is done.
>> + */
>>   struct trace_array *trace_array_create(const char *name)
>>   {
>>   	struct trace_array *tr;
>> @@ -8364,6 +8384,8 @@ struct trace_array *trace_array_create(const char *name)
>>   
>>   	list_add(&tr->list, &ftrace_trace_arrays);
>>   
>> +	tr->ref++;
>> +
>>   	mutex_unlock(&trace_types_lock);
>>   	mutex_unlock(&event_mutex);
>>   
>> @@ -8385,7 +8407,19 @@ struct trace_array *trace_array_create(const char *name)
>>   
>>   static int instance_mkdir(const char *name)
>>   {
>> -	return PTR_ERR_OR_ZERO(trace_array_create(name));
>> +	struct trace_array *tr;
>> +
>> +	tr = trace_array_create(name);
>> +	if (IS_ERR(tr))
>> +		return PTR_ERR(tr);
>> +
>> +	/* This function does not return a reference to the created trace array,
>> +	 * so the reference counter is to be 0 when it returns.
>> +	 * trace_array_create increments the ref counter, decrement it here
>> +	 * by calling trace_array_put() */
>> +	trace_array_put(tr);
>> +
> If we make it that the destroy needs tr->ref == 1, we can remove the
> above.
>
>> +	return 0;
>>   }
>>   
>>   static int __remove_instance(struct trace_array *tr)
>> @@ -8424,6 +8458,11 @@ static int __remove_instance(struct trace_array *tr)
>>   	return 0;
>>   }
>>   
>> +/*
>> + * NOTE: An instance cannot be removed if there is still a reference to it.
>> + * Make sure to call "trace_array_put" for a trace array returned by functions
>> + * like trace_array_create(), otherwise trace_array_destroy will not succeed.
>> + */
> And remove the above comment too.
>
> -- Steve
>
>>   int trace_array_destroy(struct trace_array *this_tr)
>>   {
>>   	struct trace_array *tr;

Re: [PATCH 5/5] tracing: New functions for kernel access to Ftrace instances

[Divya Indi]

Hi Steven,

Please find my comments inline -

On 10/15/19 4:19 PM, Steven Rostedt wrote:
> On Wed, 14 Aug 2019 10:55:27 -0700
> Divya Indi <divya.indi@oracle.com> 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.
>>
>> NOTE: trace_array_lookup returns a trace array and also increments the
>> reference counter associated with the returned trace array. Make sure to
>> call trace_array_put() once the use is done so that the instance can be
>> removed at a later time.
>>
>> Example use:
>>
>> tr = trace_array_lookup("foo-bar");
> Should probably be renamed to: trace_array_get_by_name("foo-bar")
>
> As the name should show that it adds a ref count.

Makes sense! Will make this change.

>
> But if we make the change I suggested before, where we do not need to
> do the put before calling the destroy, we should have:
>
>
>> if (!tr)
> {
>> 	tr = trace_array_create("foo-bar");
> 	trace_array_get(tr);

This would need a corresponding trace_array_put(?).
Additionally, this would again traverse the list of instances, So I
went with incrementing the ref ctr inside the function.

> }
>
>
>> // Log to tr
>> // Enable/disable events to tr
>> trace_array_set_clr_event(tr, _THIS_IP,"system","event",1);
> What's with the __THIS_IP?

My bad! Maybe remnants from a trace_array_printk copy-paste.
Will correct this.

>
>
>> // Done using tr
>> trace_array_put(tr);
>> ..
>>
>> Also, use trace_array_set_clr_event to enable/disable events to a trace array.
>> So now we no longer need to have ftrace_set_clr_event as an exported
>> API.
>>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> ---
>>   include/linux/trace.h        |  2 ++
>>   include/linux/trace_events.h |  3 ++-
>>   kernel/trace/trace.c         | 28 ++++++++++++++++++++++++++++
>>   kernel/trace/trace_events.c  | 23 ++++++++++++++++++++++-
>>   4 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/trace.h b/include/linux/trace.h
>> index 2c782d5..05164bb 100644
>> --- a/include/linux/trace.h
>> +++ b/include/linux/trace.h
>> @@ -32,6 +32,8 @@ 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);
>>   void trace_array_put(struct trace_array *tr);
>> +struct trace_array *trace_array_lookup(const char *name);
>> +
>>   #endif	/* CONFIG_TRACING */
>>   
>>   #endif	/* _LINUX_TRACE_H */
>> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
>> index 8a62731..05a7514 100644
>> --- a/include/linux/trace_events.h
>> +++ b/include/linux/trace_events.h
>> @@ -540,7 +540,8 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
>>   #define is_signed_type(type)	(((type)(-1)) < (type)1)
>>   
>>   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
>>    * if we try to allocate the static variable to fmt if it is not a
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 7b6a37a..e394d55 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -8514,6 +8514,34 @@ static int instance_rmdir(const char *name)
>>   	return ret;
>>   }
>>   
>> +/**
>> + * trace_array_lookup - Lookup the trace array, given its name.
>> + * @name: The name of the trace array to be looked up.
>> + *
>> + * Lookup and return the trace array associated with @name.
>> + *
>> + * NOTE: The reference counter associated with the returned trace array is
>> + * incremented to ensure it is not freed when in use. Make sure to call
>> + * "trace_array_put" for this trace array when its use is done.
>> + */
>> +struct trace_array *trace_array_lookup(const char *name)
>> +{
>> +	struct trace_array *tr;
>> +
>> +	mutex_lock(&trace_types_lock);
>> +
>> +	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
>> +		if (tr->name && strcmp(tr->name, name) == 0) {
>> +			tr->ref++;
>> +			mutex_unlock(&trace_types_lock);
>> +			return tr;
>> +		}
>> +	}
>> +	mutex_unlock(&trace_types_lock);
>> +	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 2621995..96dd997 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -834,7 +834,6 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
>>   
>>   	return ret;
>>   }
>> -EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
>>   
>>   /**
>>    * trace_set_clr_event - enable or disable an event
>> @@ -859,6 +858,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
> We probably should make this a boolean.

This would internally call ftrace_set_clr_event which would expect 0/1.
But I agree making it boolean would be more intuitive.

Thanks,
Divya

>
> -- Steve
>
>> + *
>> + * 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 -ENOENT;
>> +
>> +	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
>>   

Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Steven Rostedt]

On Wed, 16 Oct 2019 16:42:02 -0700
Divya Indi <divya.indi@oracle.com> wrote:

> Hi Steve,
> 
> Thanks again for taking the time to review and providing feedback. Please find my comments inline.
> 
> On 10/15/19 4:04 PM, Steven Rostedt wrote:
> > Sorry for taking so long to getting to these patches.
> >
> > On Wed, 14 Aug 2019 10:55:26 -0700
> > Divya Indi <divya.indi@oracle.com> wrote:
> >  
> >> For functions returning a trace array Eg: trace_array_create(), we need to
> >> increment the reference counter associated with the trace array to ensure it
> >> does not get freed when in use.
> >>
> >> Once we are done using the trace array, we need to call
> >> trace_array_put() to make sure we are not holding a reference to it
> >> anymore and the instance/trace array can be removed when required.  
> > I think it would be more in line with other parts of the kernel if we
> > don't need to do the trace_array_put() before calling
> > trace_array_destroy().  
> 
> The reason we went with this approach is
> 
> instance_mkdir -          ref_ctr = 0  // Does not return a trace array ptr.
> trace_array_create -      ref_ctr = 1  // Since this returns a trace array ptr.
> trace_array_lookup -      ref_ctr = 1  // Since this returns a trace array ptr.
> 
> if we make trace_array_destroy to expect ref_ctr to be 1, we risk destroying the trace array while in use.
> 
> We could make it -
> 
> instance_mkdir - 	ref_ctr = 1
> trace_array_create -    ref_ctr = 2
> trace_array_lookup -    ref_ctr = 2+  // depending on no of lookups
> 
> but, we'd still need the trace_array_put() (?)
> 
> We can also have one function doing create (if does not exist) or lookup (if exists), but that would require
> some redundant code since instance_mkdir needs to return -EXIST when a trace array already exists.
> 
> Let me know your thoughts on this.
> 

Can't we just move the trace_array_put() in the instance_rmdir()?

static int instance_rmdir(const char *name)
{
	struct trace_array *tr;
	int ret;

	mutex_lock(&event_mutex);
	mutex_lock(&trace_types_lock);

	ret = -ENODEV;
	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
		if (tr->name && strcmp(tr->name, name) == 0) {
			__trace_array_put(tr);
			ret = __remove_instance(tr);
			if (ret)
				tr->ref++;
			break;
		}
	}

	mutex_unlock(&trace_types_lock);
	mutex_unlock(&event_mutex);

	return ret;
}

-- Steve

Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Divya Indi]

Hi Steven,                                                                         
                                                                                 
A few clarifications on this discussion on reference counter -                   
                                                                                 
1) We will still need to export trace_array_put() to be used for every           
trace_array_get_by_name() OR trace_array_create() + trace_array_get().              
                                                                                 
How else will we reduce the reference counter [For eg: When multiple modules     
lookup the same trace array (say, reference counter = 4)]?                       
                                                                                 
2) tr = trace_array_create("my_tr");                                             
   trace_array_get(tr);                                                          
                                                                                 
Both of these functions will iterate through the list of trace arrays to verify  
whether the trace array exists (redundant, but more intuitive? Does this seem    
acceptable?)                                                                     
                                                                                 
To avoid iterating twice, we went with increasing ref_ctr in trace_array_create. 
This necessitated a trace_array_put() in instance_mkdir (Or as suggested below,
we can do this trace_array_put() in instance_rmdir().)                        
                                                                                                               
                                                                                 
3) A summary of suggested changes (Let me know if this looks good) -                                              
                                                                                 
tr = trace_array_get_by_name("foo-bar"); // ref_ctr++.                           
                                                                                 
if (!tr)                                                                         
{                                                                                
        // instance_mkdir also causes ref_ctr = 1                                
        tr = trace_array_create("foo-bar"); // ref_ctr = 1                       
        trace_array_get(tr); // ref_ctr++                                        
}                                                                                
                                                                                 
trace_array_printk(.....);                                                       
trace_array_set_clr_event(......);                                               
...                                                                              
...                                                                              
...                                                                              
// Done using the trace array.                                                   
trace_array_put(tr); // ref_ctr--                                                
...                                                                              
...                                                                              
...                                                                              
// We can now remove the trace array via trace_array_destroy or instance_rmdir()
trace_array_destroy(tr); // ref_ctr > 1 returns -EBUSY.
                                                       
                          
Thanks,                                                                          
Divya 

On 10/22/19 7:52 PM, Steven Rostedt wrote:
> On Wed, 16 Oct 2019 16:42:02 -0700
> Divya Indi <divya.indi@oracle.com> wrote:
>
>> Hi Steve,
>>
>> Thanks again for taking the time to review and providing feedback. Please find my comments inline.
>>
>> On 10/15/19 4:04 PM, Steven Rostedt wrote:
>>> Sorry for taking so long to getting to these patches.
>>>
>>> On Wed, 14 Aug 2019 10:55:26 -0700
>>> Divya Indi <divya.indi@oracle.com> wrote:
>>>  
>>>> For functions returning a trace array Eg: trace_array_create(), we need to
>>>> increment the reference counter associated with the trace array to ensure it
>>>> does not get freed when in use.
>>>>
>>>> Once we are done using the trace array, we need to call
>>>> trace_array_put() to make sure we are not holding a reference to it
>>>> anymore and the instance/trace array can be removed when required.  
>>> I think it would be more in line with other parts of the kernel if we
>>> don't need to do the trace_array_put() before calling
>>> trace_array_destroy().  
>> The reason we went with this approach is
>>
>> instance_mkdir -          ref_ctr = 0  // Does not return a trace array ptr.
>> trace_array_create -      ref_ctr = 1  // Since this returns a trace array ptr.
>> trace_array_lookup -      ref_ctr = 1  // Since this returns a trace array ptr.
>>
>> if we make trace_array_destroy to expect ref_ctr to be 1, we risk destroying the trace array while in use.
>>
>> We could make it -
>>
>> instance_mkdir - 	ref_ctr = 1
>> trace_array_create -    ref_ctr = 2
>> trace_array_lookup -    ref_ctr = 2+  // depending on no of lookups
>>
>> but, we'd still need the trace_array_put() (?)
>>
>> We can also have one function doing create (if does not exist) or lookup (if exists), but that would require
>> some redundant code since instance_mkdir needs to return -EXIST when a trace array already exists.
>>
>> Let me know your thoughts on this.
>>
> Can't we just move the trace_array_put() in the instance_rmdir()?
>
> static int instance_rmdir(const char *name)
> {
> 	struct trace_array *tr;
> 	int ret;
>
> 	mutex_lock(&event_mutex);
> 	mutex_lock(&trace_types_lock);
>
> 	ret = -ENODEV;
> 	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> 		if (tr->name && strcmp(tr->name, name) == 0) {
> 			__trace_array_put(tr);
> 			ret = __remove_instance(tr);
> 			if (ret)
> 				tr->ref++;
> 			break;
> 		}
> 	}
>
> 	mutex_unlock(&trace_types_lock);
> 	mutex_unlock(&event_mutex);
>
> 	return ret;
> }
>
> -- Steve

Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Steven Rostedt]

On Wed, 23 Oct 2019 15:57:49 -0700
Divya Indi <divya.indi@oracle.com> wrote:

> Hi Steven,                                                                         
>                                                                                  
> A few clarifications on this discussion on reference counter -                   
>                                                                                  
> 1) We will still need to export trace_array_put() to be used for every           
> trace_array_get_by_name() OR trace_array_create() + trace_array_get().        

I'm fine with exporting trace_array_put, and even trace_array_get.
      
>                                                                                  
> How else will we reduce the reference counter [For eg: When multiple modules     
> lookup the same trace array (say, reference counter = 4)]?                       
>                                                                                  
> 2) tr = trace_array_create("my_tr");                                             
>    trace_array_get(tr);                                                          
>                                                                                  
> Both of these functions will iterate through the list of trace arrays to verify  
> whether the trace array exists (redundant, but more intuitive? Does this seem    
> acceptable?)                                                                     
>                                                                                  
> To avoid iterating twice, we went with increasing ref_ctr in trace_array_create. 
> This necessitated a trace_array_put() in instance_mkdir (Or as suggested below,
> we can do this trace_array_put() in instance_rmdir().)                        
>                                                                                                                
>                                                                                  
> 3) A summary of suggested changes (Let me know if this looks good) -                                              
>                                                                                  
> tr = trace_array_get_by_name("foo-bar"); // ref_ctr++.                           
>                                                                                  
> if (!tr)                                                                         
> {                                                                                
>         // instance_mkdir also causes ref_ctr = 1                                

You'll need locking for anyone who does this, and check the return
status below for "foo-bar" existing already (due to another thread
jumping in here).

-- Steve

>         tr = trace_array_create("foo-bar"); // ref_ctr = 1                       
>         trace_array_get(tr); // ref_ctr++                                        
> }                                                                                
>                                                                                  
> trace_array_printk(.....);                                                       
> trace_array_set_clr_event(......);                                               
> ...                                                                              
> ...                                                                              
> ...                                                                              
> // Done using the trace array.                                                   
> trace_array_put(tr); // ref_ctr--                                                
> ...                                                                              
> ...                                                                              
> ...                                                                              
> // We can now remove the trace array via trace_array_destroy or instance_rmdir()
> trace_array_destroy(tr); // ref_ctr > 1 returns -EBUSY.

Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Divya Indi]

Hi Steven,

On 10/24/19 6:00 AM, Steven Rostedt wrote:
> On Wed, 23 Oct 2019 15:57:49 -0700
> Divya Indi <divya.indi@oracle.com> wrote:
>
>> Hi Steven,                                                                         
>>                                                                                  
>> A few clarifications on this discussion on reference counter -                   
>>                                                                                  
>> 1) We will still need to export trace_array_put() to be used for every           
>> trace_array_get_by_name() OR trace_array_create() + trace_array_get().        
> I'm fine with exporting trace_array_put, and even trace_array_get.
>       
>>                                                                                  
>> How else will we reduce the reference counter [For eg: When multiple modules     
>> lookup the same trace array (say, reference counter = 4)]?                       
>>                                                                                  
>> 2) tr = trace_array_create("my_tr");                                             
>>    trace_array_get(tr);                                                          
>>                                                                                  
>> Both of these functions will iterate through the list of trace arrays to verify  
>> whether the trace array exists (redundant, but more intuitive? Does this seem    
>> acceptable?)                                                                     
>>                                                                                  
>> To avoid iterating twice, we went with increasing ref_ctr in trace_array_create. 
>> This necessitated a trace_array_put() in instance_mkdir (Or as suggested below,
>> we can do this trace_array_put() in instance_rmdir().)                        
>>                                                                                                                
>>                                                                                  
>> 3) A summary of suggested changes (Let me know if this looks good) -                                              
>>                                                                                  
>> tr = trace_array_get_by_name("foo-bar"); // ref_ctr++.                           
>>                                                                                  
>> if (!tr)                                                                         
>> {                                                                                
>>         // instance_mkdir also causes ref_ctr = 1                                
> You'll need locking for anyone who does this, and check the return
> status below for "foo-bar" existing already (due to another thread
> jumping in here).

Right, Noted! Thanks for the pointer. 

>
> -- Steve
>
>>         tr = trace_array_create("foo-bar"); // ref_ctr = 1                       
>>         trace_array_get(tr); // ref_ctr++                                        
>> }                                                                                
>>                                                                                  
>> trace_array_printk(.....);                                                       
>> trace_array_set_clr_event(......);                                               
>> ...                                                                              
>> ...                                                                              
>> ...                                                                              
>> // Done using the trace array.                                                   
>> trace_array_put(tr); // ref_ctr--                                                
>> ...                                                                              
>> ...                                                                              
>> ...                                                                              
>> // We can now remove the trace array via trace_array_destroy or instance_rmdir()
>> trace_array_destroy(tr); // ref_ctr > 1 returns -EBUSY.

[PATCH 2/5] tracing: Verify if trace array exists before destroying it.

[Divya Indi]

A trace array can be destroyed from userspace or kernel. Verify if the
trace exists before proceeding to destroy/remove it.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
Reviewed-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
Reviewed-by: Manjunath Patil <manjunath.b.patil@oracle.com>
---
 kernel/trace/trace.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1c80521..bff967f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8421,17 +8421,26 @@ static int __remove_instance(struct trace_array *tr)
 	return 0;
 }
 
-int trace_array_destroy(struct trace_array *tr)
+int trace_array_destroy(struct trace_array *this_tr)
 {
+	struct trace_array *tr;
 	int ret;
 
-	if (!tr)
+	if (!this_tr)
 		return -EINVAL;
 
 	mutex_lock(&event_mutex);
 	mutex_lock(&trace_types_lock);
 
-	ret = __remove_instance(tr);
+	ret = -ENODEV;
+
+	/* Making sure trace array exists before destroying it. */
+	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+		if (tr == this_tr) {
+			ret = __remove_instance(tr);
+			break;
+		}
+	}
 
 	mutex_unlock(&trace_types_lock);
 	mutex_unlock(&event_mutex);
-- 
1.8.3.1