tracing: fix missing osnoise tracer on max_latency

PATCH Superseded Jackie Liu 2 participants 5 messages Sep 18, 2021 – Sep 22, 2021 Tracing

lore

Share & tools

lore

Jackie Liu

author

2021-09-18 05:11 UTC #0 lore raw

PATCH 1/1 tracing: fix missing osnoise tracer on max_latency

From: Jackie Liu <[email protected]>

The compiler warns when the data are actually unused:

kernel/trace/trace.c:1712:13: error: ‘trace_create_maxlat_file’ defined but not used [-Werror=unused-function]
1712 | static void trace_create_maxlat_file(struct trace_array *tr,

        |             ^~~~~~~~~~~~~~~~~~~~~~~~

[Why]
CONFIG_HWLAT_TRACER=n, CONFIG_TRACER_MAX_TRACE=n, CONFIG_OSNOISE_TRACER=y gcc report warns.

[How]
Now trace_create_maxlat_file will only take effect when CONFIG_HWLAT_TRACER=y or CONFIG_TRACER_MAX_TRACE=y. In fact, after adding osnoise trace, it also needs to take effect.

BTW, Fixed the conflicting defined comment information.

Fixes bce29ac9ce0b ("trace: Add osnoise tracer")

Cc Daniel Bristot de Oliveira <[email protected]>

Signed-off-by Jackie Liu <[email protected]>

---
 kernel/trace/trace.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

kernel/trace/trace.c +3 -6

index 7896d30d90f7..d7e3ed82fafd 100644
@@ -1744,11 +1744,7 @@ void latency_fsnotify(struct trace_array *tr)
 	irq_work_queue(&tr->fsnotify_irqwork);
 }
 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#else /* LATENCY_FS_NOTIFY */
 
 #define trace_create_maxlat_file(tr, d_tracer)				\
 	trace_create_file("tracing_max_latency", 0644, d_tracer,	\

@@ -9473,7 +9469,8 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 
 	create_trace_options_dir(tr);
 
-#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)
+#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER) \
+	|| defined(CONFIG_OSNOISE_TRACER)
 	trace_create_maxlat_file(tr, d_tracer);
 #endif

Signature · 1 line

2.25.1

Steven Rostedt

maintainer

2021-09-19 16:01 UTC #1 lore raw

Quoted 12 lines · On Sat, 18 Sep 2021 13:11:18 +0800 Jackie Liu <[email protected]> wrote:

From: Jackie Liu <[email protected]>
The compiler warns when the data are actually unused:
kernel/trace/trace.c:1712:13: error: ‘trace_create_maxlat_file’ defined but not used [-Werror=unused-function]
1712 | static void trace_create_maxlat_file(struct trace_array *tr,
        |             ^~~~~~~~~~~~~~~~~~~~~~~~
[Why]
CONFIG_HWLAT_TRACER=n, CONFIG_TRACER_MAX_TRACE=n, CONFIG_OSNOISE_TRACER=y gcc report warns.

Quoted 6 lines

[How]
Now trace_create_maxlat_file will only take effect when CONFIG_HWLAT_TRACER=y or CONFIG_TRACER_MAX_TRACE=y. In fact, after adding osnoise trace, it also needs to take effect.
BTW, Fixed the conflicting defined comment information.

Thanks for the report.

Quoted 15 lines

Fixes bce29ac9ce0b ("trace: Add osnoise tracer")
Cc Daniel Bristot de Oliveira <[email protected]>
Signed-off-by Jackie Liu <[email protected]>
---
 kernel/trace/trace.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
kernel/trace/trace.c +0 -0
index 7896d30d90f7..d7e3ed82fafd 100644
@@ -1744,11 +1744,7 @@ void latency_fsnotify(struct trace_array *tr)
 	irq_work_queue(&tr->fsnotify_irqwork);
 }

kernel/trace/trace.c void latency_fsnotify(struct trace_array *tr) · line 1750

Quoted 10 lines

kernel/trace/trace.c +1 -5

 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#else /* LATENCY_FS_NOTIFY */
 
 #define trace_create_maxlat_file(tr, d_tracer)				\
 	trace_create_file("tracing_max_latency", 0644, d_tracer,	\

To clean this up even better, we should add here:

#elif defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER) \
	|| defined(CONFIG_OSNOISE_TRACER)

#  define trace_create_maxlat_file(tr, d_tracer)			\
	trace_create_file("tracing_max_latency", 0644, d_tracer,	\
			  &tr->max_latency, &tracing_max_lat_fops)

#else

# define trace_create_maxlat_file(tr, d_tracer)	 do { } while (0)
#endif

kernel/trace/trace.c init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer) · line 9473

Quoted 7 lines

kernel/trace/trace.c +2 -1

@@ -9473,7 +9469,8 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 
 	create_trace_options_dir(tr);
 
-#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)
+#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER) \
+	|| defined(CONFIG_OSNOISE_TRACER)

And remove the #if statement complete from inside the function.

-- Steve

Quoted 3 lines

kernel/trace/trace.c +0 -0

 	trace_create_maxlat_file(tr, d_tracer);
 #endif

Jackie Liu

author

2021-09-22 02:26 UTC #2 lore raw

Hi, Steven.

Quoted 51 lines · Steven Rostedt

Quoted 12 lines · On Sat, 18 Sep 2021 13:11:18 +0800 Jackie Liu <[email protected]> wrote:
From: Jackie Liu <[email protected]>
The compiler warns when the data are actually unused:
kernel/trace/trace.c:1712:13: error: ‘trace_create_maxlat_file’ defined but not used [-Werror=unused-function]
1712 | static void trace_create_maxlat_file(struct trace_array *tr,
| ^~~~~~~~~~~~~~~~~~~~~~~~
[Why]
CONFIG_HWLAT_TRACER=n, CONFIG_TRACER_MAX_TRACE=n, CONFIG_OSNOISE_TRACER=y gcc report warns.
Quoted 6 lines
[How]
Now trace_create_maxlat_file will only take effect when CONFIG_HWLAT_TRACER=y or CONFIG_TRACER_MAX_TRACE=y. In fact, after adding osnoise trace, it also needs to take effect.
BTW, Fixed the conflicting defined comment information.
Thanks for the report.
Quoted 14 lines
Fixes bce29ac9ce0b ("trace: Add osnoise tracer")
Cc Daniel Bristot de Oliveira <[email protected]>
Signed-off-by Jackie Liu <[email protected]>
---
  kernel/trace/trace.c | 9 +++------
  1 file changed, 3 insertions(+), 6 deletions(-)
kernel/trace/trace.c +0 -0
index 7896d30d90f7..d7e3ed82fafd 100644
@@ -1744,11 +1744,7 @@ void latency_fsnotify(struct trace_array *tr)
  	irq_work_queue(&tr->fsnotify_irqwork);
kernel/trace/trace.c void latency_fsnotify(struct trace_array *tr) · line 1749
Quoted 10 lines
kernel/trace/trace.c +1 -5
  }
 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#else /* LATENCY_FS_NOTIFY  >>
  #define trace_create_maxlat_file(tr, d_tracer)				\
  	trace_create_file("tracing_max_latency", 0644, d_tracer,	\
To clean this up even better, we should add here:

Quoted 2 lines

#elif defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER) \
	|| defined(CONFIG_OSNOISE_TRACER)

This place should need to use LATENCY_FS_NOTIFY, because not only these three Traces, we also need to pay attention to CONFIG_FSNOTIFY, at least, we should not change the original meaning.

How about this:

kernel/trace/trace.c +3 -7

index 7896d30d90f7..6a88d03c6d3b 100644
@@ -1744,16 +1744,14 @@ void latency_fsnotify(struct trace_array *tr)
        irq_work_queue(&tr->fsnotify_irqwork);
 }
 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#elif defined(LATENCY_FS_NOTIFY)
 
 #define trace_create_maxlat_file(tr, d_tracer)                         \
        trace_create_file("tracing_max_latency", 0644, d_tracer,        \
                          &tr->max_latency, &tracing_max_lat_fops)
 
+#else
+#define trace_create_maxlat_file(tr, d_tracer)  do { } while (0)
 #endif
 
 #ifdef CONFIG_TRACER_MAX_TRACE

@@ -9473,9 +9471,7 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 
        create_trace_options_dir(tr);
 
-#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)
        trace_create_maxlat_file(tr, d_tracer);
-#endif
 
        if (ftrace_create_function_files(tr, d_tracer))
                MEM_FAIL(1, "Could not allocate function filter files");

==
What do you think? If there is no problem, I will send V2.

--
BR.
Jackie Liu

Quoted 25 lines

#  define trace_create_maxlat_file(tr, d_tracer)			\
	trace_create_file("tracing_max_latency", 0644, d_tracer,	\
			  &tr->max_latency, &tracing_max_lat_fops)

#else

# define trace_create_maxlat_file(tr, d_tracer)	 do { } while (0)
#endif

kernel/trace/trace.c init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer) · line 9473

Quoted 7 lines

kernel/trace/trace.c +2 -1

@@ -9473,7 +9469,8 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 
  	create_trace_options_dir(tr);
 
-#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)
+#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER) \
+	|| defined(CONFIG_OSNOISE_TRACER)

And remove the #if statement complete from inside the function.

-- Steve

Quoted 3 lines

kernel/trace/trace.c +0 -0

  	trace_create_maxlat_file(tr, d_tracer);
  #endif

Steven Rostedt

maintainer

2021-09-22 02:40 UTC #3 lore raw

Quoted 33 lines · On Wed, 22 Sep 2021 10:26:24 +0800 Jackie Liu <[email protected]> wrote:

Quoted 19 lines
Quoted 6 lines
kernel/trace/trace.c +0 -0
index 7896d30d90f7..d7e3ed82fafd 100644
@@ -1744,11 +1744,7 @@ void latency_fsnotify(struct trace_array *tr)
  	irq_work_queue(&tr->fsnotify_irqwork);
kernel/trace/trace.c void latency_fsnotify(struct trace_array *tr) · line 1749
Quoted 10 lines
kernel/trace/trace.c +1 -5
  }
 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#else /* LATENCY_FS_NOTIFY  >>
  #define trace_create_maxlat_file(tr, d_tracer)				\
  	trace_create_file("tracing_max_latency", 0644, d_tracer,	\  
To clean this up even better, we should add here:
Quoted 2 lines
#elif defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER) \
	|| defined(CONFIG_OSNOISE_TRACER)  
This place should need to use LATENCY_FS_NOTIFY, because not only these three Traces, we also need to pay attention to CONFIG_FSNOTIFY, at least, we should not change the original meaning.
How about this:
kernel/trace/trace.c +0 -0
index 7896d30d90f7..6a88d03c6d3b 100644

kernel/trace/trace.c void latency_fsnotify(struct trace_array *tr) · line 1747

Quoted 9 lines

kernel/trace/trace.c +1 -5

@@ -1744,16 +1744,14 @@ void latency_fsnotify(struct trace_array *tr)
         irq_work_queue(&tr->fsnotify_irqwork);
  }
 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#elif defined(LATENCY_FS_NOTIFY)

Um, but isn't the #if before the #else:

  #ifdef LATENCY_FS_NOTIFY

Then, here we have:

#ifdef LATENCY_FS_NOTIFY

[..]

#elif defined(LATENCY_FS_NOTIFY)

// this will never be called.

That doesn't make any sense.

-- Steve

Quoted 26 lines

kernel/trace/trace.c +2 -2

  #define trace_create_maxlat_file(tr, d_tracer)                         \
         trace_create_file("tracing_max_latency", 0644, d_tracer,        \
                           &tr->max_latency, &tracing_max_lat_fops)
 
+#else
+#define trace_create_maxlat_file(tr, d_tracer)  do { } while (0)
  #endif
 
  #ifdef CONFIG_TRACER_MAX_TRACE

@@ -9473,9 +9471,7 @@ init_tracer_tracefs(struct trace_array *tr, struct 
 dentry *d_tracer)
 
         create_trace_options_dir(tr);
 
-#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)
         trace_create_maxlat_file(tr, d_tracer);
-#endif
 
         if (ftrace_create_function_files(tr, d_tracer))

                 MEM_FAIL(1, "Could not allocate function filter files");

==
What do you think? If there is no problem, I will send V2.

Jackie Liu

author

2021-09-22 02:45 UTC #4 lore raw

Quoted 59 lines · Steven Rostedt

Quoted 33 lines · On Wed, 22 Sep 2021 10:26:24 +0800 Jackie Liu <[email protected]> wrote:
Quoted 19 lines
Quoted 6 lines
kernel/trace/trace.c +0 -0
index 7896d30d90f7..d7e3ed82fafd 100644
@@ -1744,11 +1744,7 @@ void latency_fsnotify(struct trace_array *tr)
   	irq_work_queue(&tr->fsnotify_irqwork);
kernel/trace/trace.c void latency_fsnotify(struct trace_array *tr) · line 1749
Quoted 10 lines
kernel/trace/trace.c +1 -5
   }
 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#else /* LATENCY_FS_NOTIFY  >>
   #define trace_create_maxlat_file(tr, d_tracer)				\
   	trace_create_file("tracing_max_latency", 0644, d_tracer,	\
To clean this up even better, we should add here:
Quoted 2 lines
#elif defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER) \
	|| defined(CONFIG_OSNOISE_TRACER)
This place should need to use LATENCY_FS_NOTIFY, because not only these three Traces, we also need to pay attention to CONFIG_FSNOTIFY, at least, we should not change the original meaning.
How about this:
kernel/trace/trace.c +0 -0
index 7896d30d90f7..6a88d03c6d3b 100644
kernel/trace/trace.c void latency_fsnotify(struct trace_array *tr) · line 1747
Quoted 9 lines
kernel/trace/trace.c +1 -5
@@ -1744,16 +1744,14 @@ void latency_fsnotify(struct trace_array *tr)
          irq_work_queue(&tr->fsnotify_irqwork);
   }
 
-/*
- * (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
- *  defined(CONFIG_FSNOTIFY)
- */
-#else
+#elif defined(LATENCY_FS_NOTIFY)
Um, but isn't the #if before the #else:
   #ifdef LATENCY_FS_NOTIFY
?
Then, here we have:
#ifdef LATENCY_FS_NOTIFY
[..]

Quoted 7 lines

#elif defined(LATENCY_FS_NOTIFY)
// this will never be called.
That doesn't make any sense.
-- Steve

Thanks Steve, I see. :)

Signature · 1 line

BR, Jackie Liu

Quoted 27 lines

kernel/trace/trace.c +2 -2

   #define trace_create_maxlat_file(tr, d_tracer)                         \
          trace_create_file("tracing_max_latency", 0644, d_tracer,        \
                            &tr->max_latency, &tracing_max_lat_fops)
 
+#else
+#define trace_create_maxlat_file(tr, d_tracer)  do { } while (0)
   #endif
 
   #ifdef CONFIG_TRACER_MAX_TRACE

@@ -9473,9 +9471,7 @@ init_tracer_tracefs(struct trace_array *tr, struct
 dentry *d_tracer)
 
          create_trace_options_dir(tr);
 
-#if defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)
          trace_create_maxlat_file(tr, d_tracer);
-#endif
 
          if (ftrace_create_function_files(tr, d_tracer))

                  MEM_FAIL(1, "Could not allocate function filter files");

==
What do you think? If there is no problem, I will send V2.