[RFC][v2] Kernel Access to Ftrace Instances.

[RFC][v2] Kernel Access to Ftrace Instances.

[Divya Indi]

Hi,
Please review the patches that follow -

[PATCH 1/3] tracing: Relevant changes for kernel access to Ftrace instances.
[PATCH 2/3] tracing: Adding additional NULL checks.
[PATCH 3/3] tracing: Add 2 new funcs. for kernel access to Ftrace instances.

This is v2 for the series with changes made to Patch 3/3 to address review comments. 

The new changes ensure that a trace array created by trace_array_create or
accessed via trace_array_lookup cannot be freed if still in use.

Let me know if you have any questions/concerns/suggestions. 

Thanks,
Divya 

[PATCH 1/3] tracing: Relevant changes for kernel access to Ftrace instances.

[Divya Indi]

For commit f45d1225adb0 ("tracing: Kernel access to Ftrace instances")
Adding the following changes to ensure other kernel components can
use these functions -
1) Remove static keyword for newly exported fn - ftrace_set_clr_event.
2) Add the req functions to header file include/linux/trace_events.h.

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

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 8a62731..d7b7d85 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -539,6 +539,12 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
 
 #define is_signed_type(type)	(((type)(-1)) < (type)1)
 
+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);
+int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
 int trace_set_clr_event(const char *system, const char *event, int set);
 
 /*
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 0ce3db6..b6b4618 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -795,7 +795,7 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
 	return ret;
 }
 
-static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
+int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
 {
 	char *event = NULL, *sub = NULL, *match;
 	int ret;
-- 
1.8.3.1

[PATCH 2/3] tracing: Adding additional NULL checks.

[Divya Indi]

commit f45d1225adb0 ("tracing: Kernel access to Ftrace instances")
exported certain functions providing access to Ftrace instances from
other kernel components.
Adding some additional NULL checks to ensure safe usage by the users.

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 1c80521..a60dc13 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 -EINVAL;
+
 	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 b6b4618..445b059 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -800,6 +800,8 @@ int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
 	char *event = NULL, *sub = NULL, *match;
 	int ret;
 
+	if (!tr)
+		return -ENODEV;
 	/*
 	 * The buf format can be <subsystem>:<event-name>
 	 *  *:<event-name> means any event by that name.
-- 
1.8.3.1

[PATCH 3/3] tracing: Add 2 new funcs. 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.

Newly added functions trace_array_lookup and trace_array_create also
need to increment the reference counter associated with the trace array
they return. This is to ensure the trace array does not get freed
while in use by the newly introduced APIs.
The reference ctr is decremented in the trace_array_destroy.

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

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..fb70ccc 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8364,6 +8364,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 +8387,14 @@ 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);
+	trace_array_put(tr);
+
+	return 0;
 }
 
 static int __remove_instance(struct trace_array *tr)
@@ -8434,6 +8443,7 @@ int trace_array_destroy(struct trace_array *tr)
 	mutex_lock(&event_mutex);
 	mutex_lock(&trace_types_lock);
 
+	tr->ref--;
 	ret = __remove_instance(tr);
 
 	mutex_unlock(&trace_types_lock);
@@ -8465,6 +8475,24 @@ static int instance_rmdir(const char *name)
 	return ret;
 }
 
+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 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
 
-- 
1.8.3.1

Re: [PATCH 1/3] tracing: Relevant changes for kernel access to Ftrace instances.

[Steven Rostedt]

On Wed, 12 Jun 2019 09:34:17 -0700
Divya Indi <divya.indi@oracle.com> wrote:

Hi Divya,

First, make sure the patches are all replied to the cover patch [0/3].
That is, patch 1, 2 and 3 should all be in reply to [0/3] to see a all
the patches at the same level, and that makes replies easy to stand out.

Having patch 2 a reply to patch 1 and patch 3 a reply to patch 2, makes
it hard to see what comments are for what patches.

> For commit f45d1225adb0 ("tracing: Kernel access to Ftrace instances")
> Adding the following changes to ensure other kernel components can
> use these functions -
> 1) Remove static keyword for newly exported fn - ftrace_set_clr_event.
> 2) Add the req functions to header file include/linux/trace_events.h.

The above change log is hard to parse. The "Adding" looks to be a start
of a new sentence, and what changes and what components can use these
functions?

Also avoid shorten words ("fn", "req") that just makes trying to figure
out what is being said that more confusing.

> 
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> ---
>  include/linux/trace_events.h | 6 ++++++
>  kernel/trace/trace_events.c  | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index 8a62731..d7b7d85 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -539,6 +539,12 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
>  
>  #define is_signed_type(type)	(((type)(-1)) < (type)1)
>  
> +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);
> +int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);

These function are declared elsewhere. If we are adding them here, then
they should be removed from the other places.

But trace_events.h is not the proper place to put these. The
trace_events.h file is only for code used in the TRACE_EVENT() macros.

The proper file is include/linux/trace.h

-- Steve

>  int trace_set_clr_event(const char *system, const char *event, int set);
>  
>  /*
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 0ce3db6..b6b4618 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -795,7 +795,7 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
>  	return ret;
>  }
>  
> -static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> +int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
>  {
>  	char *event = NULL, *sub = NULL, *match;
>  	int ret;

Re: [PATCH 2/3] tracing: Adding additional NULL checks.

[Steven Rostedt]

On Wed, 12 Jun 2019 09:34:18 -0700
Divya Indi <divya.indi@oracle.com> wrote:

> commit f45d1225adb0 ("tracing: Kernel access to Ftrace instances")
> exported certain functions providing access to Ftrace instances from
> other kernel components.

I'm fine with the patch, the above statement is hard to understand.

-- Steve

> Adding some additional NULL checks to ensure safe usage by the users.
> 
> 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 1c80521..a60dc13 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 -EINVAL;
> +
>  	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 b6b4618..445b059 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -800,6 +800,8 @@ int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
>  	char *event = NULL, *sub = NULL, *match;
>  	int ret;
>  
> +	if (!tr)
> +		return -ENODEV;
>  	/*
>  	 * The buf format can be <subsystem>:<event-name>
>  	 *  *:<event-name> means any event by that name.

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

[Steven Rostedt]

On Wed, 12 Jun 2019 09:34:19 -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.
> 
> Newly added functions trace_array_lookup and trace_array_create also
> need to increment the reference counter associated with the trace array
> they return. This is to ensure the trace array does not get freed
> while in use by the newly introduced APIs.
> The reference ctr is decremented in the trace_array_destroy.
> 
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> ---
>  include/linux/trace_events.h |  3 +++
>  kernel/trace/trace.c         | 30 +++++++++++++++++++++++++++++-
>  kernel/trace/trace_events.c  | 22 ++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 1 deletion(-)
> 
> 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..fb70ccc 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -8364,6 +8364,8 @@ struct trace_array *trace_array_create(const char *name)
>  
>  	list_add(&tr->list, &ftrace_trace_arrays);
>  
> +	tr->ref++;
> +

Needs a comment for why the ref is incremented.


>  	mutex_unlock(&trace_types_lock);
>  	mutex_unlock(&event_mutex);
>  
> @@ -8385,7 +8387,14 @@ 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);
> +	trace_array_put(tr);

Need a comment to why we need to do a put here.

> +
> +	return 0;
>  }
>  
>  static int __remove_instance(struct trace_array *tr)
> @@ -8434,6 +8443,7 @@ int trace_array_destroy(struct trace_array *tr)
>  	mutex_lock(&event_mutex);
>  	mutex_lock(&trace_types_lock);
>  
> +	tr->ref--;
>  	ret = __remove_instance(tr);
>  
>  	mutex_unlock(&trace_types_lock);
> @@ -8465,6 +8475,24 @@ static int instance_rmdir(const char *name)
>  	return ret;
>  }
>  

Need a kerneldoc heading here, that also states that if a trace_array
is returned, it requires a call to trace_array_put().


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

If we have this, should we get rid of the external use of
ftrace_set_clr_event()?

-- Steve

> +
> +	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 3/3] tracing: Add 2 new funcs. for kernel access to Ftrace instances.

[Divya Indi]

Hi Steven,

Thanks for reviewing the patch. I have made a note of all the suggested 
changes, comment requirements.

Will send them out soon as v2.

(comments inline)

On 6/17/19 4:45 PM, Steven Rostedt wrote:
> On Wed, 12 Jun 2019 09:34:19 -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.
>>
>> Newly added functions trace_array_lookup and trace_array_create also
>> need to increment the reference counter associated with the trace array
>> they return. This is to ensure the trace array does not get freed
>> while in use by the newly introduced APIs.
>> The reference ctr is decremented in the trace_array_destroy.
>>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> ---
>>   include/linux/trace_events.h |  3 +++
>>   kernel/trace/trace.c         | 30 +++++++++++++++++++++++++++++-
>>   kernel/trace/trace_events.c  | 22 ++++++++++++++++++++++
>>   3 files changed, 54 insertions(+), 1 deletion(-)
>>
>> 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..fb70ccc 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -8364,6 +8364,8 @@ struct trace_array *trace_array_create(const char *name)
>>   
>>   	list_add(&tr->list, &ftrace_trace_arrays);
>>   
>> +	tr->ref++;
>> +
> Needs a comment for why the ref is incremented.
>
>
>>   	mutex_unlock(&trace_types_lock);
>>   	mutex_unlock(&event_mutex);
>>   
>> @@ -8385,7 +8387,14 @@ 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);
>> +	trace_array_put(tr);
> Need a comment to why we need to do a put here.
>
>> +
>> +	return 0;
>>   }
>>   
>>   static int __remove_instance(struct trace_array *tr)
>> @@ -8434,6 +8443,7 @@ int trace_array_destroy(struct trace_array *tr)
>>   	mutex_lock(&event_mutex);
>>   	mutex_lock(&trace_types_lock);
>>   
>> +	tr->ref--;
>>   	ret = __remove_instance(tr);
>>   
>>   	mutex_unlock(&trace_types_lock);
>> @@ -8465,6 +8475,24 @@ static int instance_rmdir(const char *name)
>>   	return ret;
>>   }
>>   
> Need a kerneldoc heading here, that also states that if a trace_array
> is returned, it requires a call to trace_array_put().
>
>
>> +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 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;
> If we have this, should we get rid of the external use of
> ftrace_set_clr_event()?

I think so too. External use of trace_array_set_clr_event is more 
convenient and intuitive. Will make the change.


Thanks,

Divya

>
> -- Steve
>
>> +
>> +	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 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@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.
>>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> ---
>>   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
>>   

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

[Steven Rostedt]

On Tue,  4 Jun 2019 17:42:05 -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.
> 
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> ---
>  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


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

[PATCH 3/3] tracing: Add 2 new funcs. 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.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
---
 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) {
+		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
 
-- 
1.8.3.1