diff mbox series

[1/2] tracing: Remove unnecessary copying of tr->current_trace

Message ID 20230713114626.704ebe8e@gandalf.local.home
State New
Headers
Series tracing: Clean up how iter is freed |

Commit Message

Steven Rostedt July 13, 2023, 3:46 p.m. UTC 
  From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

I have no idea why the iterator had to allocate a descriptor to make a
copy of "current_trace" (now "tr->current_trace"). The content of that
pointer never changes, so it's sufficient to just copy the pointer to
maintain integrity with reading it.

This is more of a clean up than a fix.

Fixes: d7350c3f45694 ("tracing/core: make the read callbacks reentrants")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

Comments

Masami Hiramatsu (Google) July 14, 2023, 8:38 a.m. UTC | #1 
On Thu, 13 Jul 2023 11:46:26 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> I have no idea why the iterator had to allocate a descriptor to make a
> copy of "current_trace" (now "tr->current_trace"). The content of that
> pointer never changes, so it's sufficient to just copy the pointer to
> maintain integrity with reading it.
> 
> This is more of a clean up than a fix.

Interesting bug. Anyway looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

> 
> Fixes: d7350c3f45694 ("tracing/core: make the read callbacks reentrants")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index be847d45d81c..1c370ffbe062 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4205,15 +4205,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
>  	loff_t l = 0;
>  	int cpu;
>  
> -	/*
> -	 * copy the tracer to avoid using a global lock all around.
> -	 * iter->trace is a copy of current_trace, the pointer to the
> -	 * name may be used instead of a strcmp(), as iter->trace->name
> -	 * will point to the same string as current_trace->name.
> -	 */
>  	mutex_lock(&trace_types_lock);
> -	if (unlikely(tr->current_trace && iter->trace->name != tr->current_trace->name))
> -		*iter->trace = *tr->current_trace;
> +	if (unlikely(tr->current_trace != iter->trace))
> +		iter->trace = tr->current_trace;
>  	mutex_unlock(&trace_types_lock);
>  
>  #ifdef CONFIG_TRACER_MAX_TRACE
> @@ -4862,16 +4856,8 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  	iter->fmt = NULL;
>  	iter->fmt_size = 0;
>  
> -	/*
> -	 * We make a copy of the current tracer to avoid concurrent
> -	 * changes on it while we are reading.
> -	 */
>  	mutex_lock(&trace_types_lock);
> -	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
> -	if (!iter->trace)
> -		goto fail;
> -
> -	*iter->trace = *tr->current_trace;
> +	iter->trace = tr->current_trace;
>  
>  	if (!zalloc_cpumask_var(&iter->started, GFP_KERNEL))
>  		goto fail;
> @@ -4936,7 +4922,6 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  
>   fail:
>  	mutex_unlock(&trace_types_lock);
> -	kfree(iter->trace);
>  	kfree(iter->temp);
>  	kfree(iter->buffer_iter);
>  release:
> @@ -5021,7 +5006,6 @@ static int tracing_release(struct inode *inode, struct file *file)
>  	free_cpumask_var(iter->started);
>  	kfree(iter->fmt);
>  	kfree(iter->temp);
> -	kfree(iter->trace);
>  	kfree(iter->buffer_iter);
>  	seq_release_private(inode, file);
>  
> -- 
> 2.40.1
>
diff mbox series

Patch

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index be847d45d81c..1c370ffbe062 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4205,15 +4205,9 @@  static void *s_start(struct seq_file *m, loff_t *pos)
 	loff_t l = 0;
 	int cpu;
 
-	/*
-	 * copy the tracer to avoid using a global lock all around.
-	 * iter->trace is a copy of current_trace, the pointer to the
-	 * name may be used instead of a strcmp(), as iter->trace->name
-	 * will point to the same string as current_trace->name.
-	 */
 	mutex_lock(&trace_types_lock);
-	if (unlikely(tr->current_trace && iter->trace->name != tr->current_trace->name))
-		*iter->trace = *tr->current_trace;
+	if (unlikely(tr->current_trace != iter->trace))
+		iter->trace = tr->current_trace;
 	mutex_unlock(&trace_types_lock);
 
 #ifdef CONFIG_TRACER_MAX_TRACE
@@ -4862,16 +4856,8 @@  __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 	iter->fmt = NULL;
 	iter->fmt_size = 0;
 
-	/*
-	 * We make a copy of the current tracer to avoid concurrent
-	 * changes on it while we are reading.
-	 */
 	mutex_lock(&trace_types_lock);
-	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
-	if (!iter->trace)
-		goto fail;
-
-	*iter->trace = *tr->current_trace;
+	iter->trace = tr->current_trace;
 
 	if (!zalloc_cpumask_var(&iter->started, GFP_KERNEL))
 		goto fail;
@@ -4936,7 +4922,6 @@  __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 
  fail:
 	mutex_unlock(&trace_types_lock);
-	kfree(iter->trace);
 	kfree(iter->temp);
 	kfree(iter->buffer_iter);
 release:
@@ -5021,7 +5006,6 @@  static int tracing_release(struct inode *inode, struct file *file)
 	free_cpumask_var(iter->started);
 	kfree(iter->fmt);
 	kfree(iter->temp);
-	kfree(iter->trace);
 	kfree(iter->buffer_iter);
 	seq_release_private(inode, file);