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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Thanks,
Aruna
Sorry for taking so long to getting to these patches.
On Wed, 14 Aug 2019 10:55:26 -0700
Divya Indi <[email protected]> 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 <[email protected]>
> ---
> 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;
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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;
On Wed, 16 Oct 2019 16:42:02 -0700
Divya Indi <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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
On Wed, 23 Oct 2019 15:57:49 -0700
Divya Indi <[email protected]> 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.
Hi Steven,
On 10/24/19 6:00 AM, Steven Rostedt wrote:
> On Wed, 23 Oct 2019 15:57:49 -0700
> Divya Indi <[email protected]> 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.