Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions
From: Divya Indi
Date: Wed Oct 16 2019 - 19:43:56 EST
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@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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;