From c1fa617caeb005e7e3db60826cff6dddebb0363f Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Thu, 22 Feb 2024 16:14:16 -0500 Subject: tracing: Rework __assign_str() and __string() to not duplicate getting the string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TRACE_EVENT() macro handles dynamic strings by having: TP_PROTO(struct some_struct *s), TP_ARGS(s), TP_STRUCT__entry( __string(my_string, s->string) ), TP_fast_assign( __assign_str(my_string, s->string); ) TP_printk("%s", __get_str(my_string)) There's even some code that may call a function helper to find the s->string value. The problem with the above is that the work to get the s->string is done twice. Once at the __string() and again in the __assign_str(). But the __string() uses dynamic_array() which has a helper structure that is created holding the offsets and length of the string fields. Instead of finding the string twice, just save it off in another field from that helper structure, and have __assign_str() use that instead. Note, this also means that the second parameter of __assign_str() isn't even used anymore, and may be removed in the future. Link: https://lore.kernel.org/linux-trace-kernel/20240222211442.634192653@goodmis.org Cc: Masami Hiramatsu Cc: Mark Rutland Cc: Mathieu Desnoyers Cc: Andrew Morton Cc: Ville Syrjälä Cc: Rodrigo Vivi Cc: Chuck Lever Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage2_data_offsets.h | 4 ++-- include/trace/stages/stage5_get_offsets.h | 15 ++++++++++----- include/trace/stages/stage6_event_callback.h | 12 ++++++++---- 3 files changed, 20 insertions(+), 11 deletions(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage2_data_offsets.h b/include/trace/stages/stage2_data_offsets.h index 469b6a64293d..8b0cff06d346 100644 --- a/include/trace/stages/stage2_data_offsets.h +++ b/include/trace/stages/stage2_data_offsets.h @@ -24,7 +24,7 @@ #define __array(type, item, len) #undef __dynamic_array -#define __dynamic_array(type, item, len) u32 item; +#define __dynamic_array(type, item, len) u32 item; const void *item##_ptr_; #undef __string #define __string(item, src) __dynamic_array(char, item, -1) @@ -45,7 +45,7 @@ #define __sockaddr(field, len) __dynamic_array(u8, field, len) #undef __rel_dynamic_array -#define __rel_dynamic_array(type, item, len) u32 item; +#define __rel_dynamic_array(type, item, len) u32 item; const void *item##_ptr_; #undef __rel_string #define __rel_string(item, src) __rel_dynamic_array(char, item, -1) diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h index e30a13be46ba..45f8151cf622 100644 --- a/include/trace/stages/stage5_get_offsets.h +++ b/include/trace/stages/stage5_get_offsets.h @@ -47,10 +47,12 @@ #undef __string #define __string(item, src) __dynamic_array(char, item, \ - strlen((src) ? (const char *)(src) : "(null)") + 1) + strlen((src) ? (const char *)(src) : "(null)") + 1) \ + __data_offsets->item##_ptr_ = src; #undef __string_len -#define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1) +#define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)\ + __data_offsets->item##_ptr_ = src; #undef __vstring #define __vstring(item, fmt, ap) __dynamic_array(char, item, \ @@ -67,11 +69,14 @@ __data_size += __item_length; #undef __rel_string -#define __rel_string(item, src) __rel_dynamic_array(char, item, \ - strlen((src) ? (const char *)(src) : "(null)") + 1) +#define __rel_string(item, src) __rel_dynamic_array(char, item, \ + strlen((src) ? (const char *)(src) : "(null)") + 1) \ + __data_offsets->item##_ptr_ = src; #undef __rel_string_len -#define __rel_string_len(item, src, len) __rel_dynamic_array(char, item, (len) + 1) +#define __rel_string_len(item, src, len) __rel_dynamic_array(char, item, (len) + 1)\ + __data_offsets->item##_ptr_ = src; + /* * __bitmask_size_in_bytes_raw is the number of bytes needed to hold * num_possible_cpus(). diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 919b1a4da980..b3e2f321e787 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -32,12 +32,14 @@ #undef __assign_str #define __assign_str(dst, src) \ - strcpy(__get_str(dst), (src) ? (const char *)(src) : "(null)"); + strcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? \ + __data_offsets.dst##_ptr_ : "(null)") #undef __assign_str_len #define __assign_str_len(dst, src, len) \ do { \ - memcpy(__get_str(dst), (src), (len)); \ + memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? \ + __data_offsets.dst##_ptr_ : "(null)", len); \ __get_str(dst)[len] = '\0'; \ } while(0) @@ -92,12 +94,14 @@ #undef __assign_rel_str #define __assign_rel_str(dst, src) \ - strcpy(__get_rel_str(dst), (src) ? (const char *)(src) : "(null)"); + strcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? \ + __data_offsets.dst##_ptr_ : "(null)") #undef __assign_rel_str_len #define __assign_rel_str_len(dst, src, len) \ do { \ - memcpy(__get_rel_str(dst), (src), (len)); \ + memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? \ + __data_offsets.dst##_ptr_ : "(null)", len); \ __get_rel_str(dst)[len] = '\0'; \ } while (0) -- cgit v1.2.3 From e8b737bfb16a0d540413173e8d1574e3bf8cc0e9 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Thu, 22 Feb 2024 16:14:17 -0500 Subject: tracing: Do not calculate strlen() twice for __string() fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TRACE_EVENT() macro handles dynamic strings by having: TP_PROTO(struct some_struct *s), TP_ARGS(s), TP_STRUCT__entry( __string(my_string, s->string) ), TP_fast_assign( __assign_str(my_string, s->string); ) TP_printk("%s", __get_str(my_string)) There's even some code that may call a function helper to find the s->string value. The problem with the above is that the work to get the s->string is done twice. Once at the __string() and again in the __assign_str(). The length of the string is calculated via a strlen(), not once, but twice. Once during the __string() macro and again in __assign_str(). But the length is actually already recorded in the data location and here's no reason to call strlen() again. Just use the saved length that was saved in the __string() code for the __assign_str() code. Link: https://lore.kernel.org/linux-trace-kernel/20240222211442.793074999@goodmis.org Cc: Masami Hiramatsu Cc: Mark Rutland Cc: Mathieu Desnoyers Cc: Andrew Morton Cc: Ville Syrjälä Cc: Rodrigo Vivi Cc: Chuck Lever Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage6_event_callback.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index b3e2f321e787..c0e5d097324e 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -32,8 +32,9 @@ #undef __assign_str #define __assign_str(dst, src) \ - strcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? \ - __data_offsets.dst##_ptr_ : "(null)") + memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? \ + __data_offsets.dst##_ptr_ : "(null)", \ + __get_dynamic_array_len(dst)) #undef __assign_str_len #define __assign_str_len(dst, src, len) \ @@ -94,8 +95,9 @@ #undef __assign_rel_str #define __assign_rel_str(dst, src) \ - strcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? \ - __data_offsets.dst##_ptr_ : "(null)") + memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? \ + __data_offsets.dst##_ptr_ : "(null)", \ + __get_rel_dynamic_array_len(dst)) #undef __assign_rel_str_len #define __assign_rel_str_len(dst, src, len) \ -- cgit v1.2.3 From 916849860fa9c7d3caeb144cb5dec8831cf23bfc Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Thu, 22 Feb 2024 16:14:18 -0500 Subject: tracing: Use ? : shortcut in trace macros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of having: #define __assign_str(dst, src) \ memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? \ __data_offsets.dst##_ptr_ : "(null)", \ __get_dynamic_array_len(dst)) Use the ? : shortcut and compact it down to: #define __assign_str(dst, src) \ memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? : "(null)", \ __get_dynamic_array_len(dst)) Link: https://lore.kernel.org/linux-trace-kernel/20240222211442.949327725@goodmis.org Cc: Masami Hiramatsu Cc: Mark Rutland Cc: Andrew Morton Cc: Ville Syrjälä Cc: Rodrigo Vivi Cc: Chuck Lever Suggested-by: Mathieu Desnoyers Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage5_get_offsets.h | 4 ++-- include/trace/stages/stage6_event_callback.h | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h index 45f8151cf622..20b801ed3fd4 100644 --- a/include/trace/stages/stage5_get_offsets.h +++ b/include/trace/stages/stage5_get_offsets.h @@ -47,7 +47,7 @@ #undef __string #define __string(item, src) __dynamic_array(char, item, \ - strlen((src) ? (const char *)(src) : "(null)") + 1) \ + strlen((const char *)(src) ? : "(null)") + 1) \ __data_offsets->item##_ptr_ = src; #undef __string_len @@ -70,7 +70,7 @@ #undef __rel_string #define __rel_string(item, src) __rel_dynamic_array(char, item, \ - strlen((src) ? (const char *)(src) : "(null)") + 1) \ + strlen((const char *)(src) ? : "(null)") + 1) \ __data_offsets->item##_ptr_ = src; #undef __rel_string_len diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index c0e5d097324e..38732855eadb 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -32,15 +32,14 @@ #undef __assign_str #define __assign_str(dst, src) \ - memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? \ - __data_offsets.dst##_ptr_ : "(null)", \ + memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? : "(null)", \ __get_dynamic_array_len(dst)) #undef __assign_str_len #define __assign_str_len(dst, src, len) \ do { \ - memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? \ - __data_offsets.dst##_ptr_ : "(null)", len); \ + memcpy(__get_str(dst), \ + __data_offsets.dst##_ptr_ ? : "(null)", len); \ __get_str(dst)[len] = '\0'; \ } while(0) @@ -95,15 +94,14 @@ #undef __assign_rel_str #define __assign_rel_str(dst, src) \ - memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? \ - __data_offsets.dst##_ptr_ : "(null)", \ + memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? : "(null)", \ __get_rel_dynamic_array_len(dst)) #undef __assign_rel_str_len #define __assign_rel_str_len(dst, src, len) \ do { \ - memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? \ - __data_offsets.dst##_ptr_ : "(null)", len); \ + memcpy(__get_rel_str(dst), \ + __data_offsets.dst##_ptr_ ? : "(null)", len); \ __get_rel_str(dst)[len] = '\0'; \ } while (0) -- cgit v1.2.3 From 70a6ed553f7d3504febac467cb4a0bae621ba3c6 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Thu, 22 Feb 2024 16:14:19 -0500 Subject: tracing: Use EVENT_NULL_STR macro instead of open coding "(null)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TRACE_EVENT macros has some dependency if a __string() field is NULL, where it will save "(null)" as the string. This string is also used by __assign_str(). It's better to create a single macro instead of having something that will not be caught by the compiler if there is an unfortunate typo. Link: https://lore.kernel.org/linux-trace-kernel/20240222211443.106216915@goodmis.org Cc: Masami Hiramatsu Cc: Mark Rutland Cc: Andrew Morton Cc: Ville Syrjälä Cc: Rodrigo Vivi Cc: Chuck Lever Suggested-by: Mathieu Desnoyers Signed-off-by: Steven Rostedt (Google) --- include/trace/events/sunrpc.h | 12 ++++++------ include/trace/stages/stage5_get_offsets.h | 4 ++-- include/trace/stages/stage6_event_callback.h | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) (limited to 'include/trace') diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h index cdd3a45e6003..ce6a85b82afa 100644 --- a/include/trace/events/sunrpc.h +++ b/include/trace/events/sunrpc.h @@ -1327,18 +1327,18 @@ TRACE_EVENT(xs_stream_read_data, __field(ssize_t, err) __field(size_t, total) __string(addr, xprt ? xprt->address_strings[RPC_DISPLAY_ADDR] : - "(null)") + EVENT_NULL_STR) __string(port, xprt ? xprt->address_strings[RPC_DISPLAY_PORT] : - "(null)") + EVENT_NULL_STR) ), TP_fast_assign( __entry->err = err; __entry->total = total; __assign_str(addr, xprt ? - xprt->address_strings[RPC_DISPLAY_ADDR] : "(null)"); + xprt->address_strings[RPC_DISPLAY_ADDR] : EVENT_NULL_STR); __assign_str(port, xprt ? - xprt->address_strings[RPC_DISPLAY_PORT] : "(null)"); + xprt->address_strings[RPC_DISPLAY_PORT] : EVENT_NULL_STR); ), TP_printk("peer=[%s]:%s err=%zd total=%zu", __get_str(addr), @@ -1783,7 +1783,7 @@ TRACE_EVENT(svc_process, __string(service, name) __string(procedure, svc_proc_name(rqst)) __string(addr, rqst->rq_xprt ? - rqst->rq_xprt->xpt_remotebuf : "(null)") + rqst->rq_xprt->xpt_remotebuf : EVENT_NULL_STR) ), TP_fast_assign( @@ -1793,7 +1793,7 @@ TRACE_EVENT(svc_process, __assign_str(service, name); __assign_str(procedure, svc_proc_name(rqst)); __assign_str(addr, rqst->rq_xprt ? - rqst->rq_xprt->xpt_remotebuf : "(null)"); + rqst->rq_xprt->xpt_remotebuf : EVENT_NULL_STR); ), TP_printk("addr=%s xid=0x%08x service=%s vers=%u proc=%s", diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h index 20b801ed3fd4..e6b96608f452 100644 --- a/include/trace/stages/stage5_get_offsets.h +++ b/include/trace/stages/stage5_get_offsets.h @@ -47,7 +47,7 @@ #undef __string #define __string(item, src) __dynamic_array(char, item, \ - strlen((const char *)(src) ? : "(null)") + 1) \ + strlen((const char *)(src) ? : EVENT_NULL_STR) + 1) \ __data_offsets->item##_ptr_ = src; #undef __string_len @@ -70,7 +70,7 @@ #undef __rel_string #define __rel_string(item, src) __rel_dynamic_array(char, item, \ - strlen((const char *)(src) ? : "(null)") + 1) \ + strlen((const char *)(src) ? : EVENT_NULL_STR) + 1) \ __data_offsets->item##_ptr_ = src; #undef __rel_string_len diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 38732855eadb..2bfd49713b42 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -32,14 +32,14 @@ #undef __assign_str #define __assign_str(dst, src) \ - memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? : "(null)", \ + memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, \ __get_dynamic_array_len(dst)) #undef __assign_str_len #define __assign_str_len(dst, src, len) \ do { \ memcpy(__get_str(dst), \ - __data_offsets.dst##_ptr_ ? : "(null)", len); \ + __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, len); \ __get_str(dst)[len] = '\0'; \ } while(0) @@ -94,14 +94,14 @@ #undef __assign_rel_str #define __assign_rel_str(dst, src) \ - memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? : "(null)", \ + memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, \ __get_rel_dynamic_array_len(dst)) #undef __assign_rel_str_len #define __assign_rel_str_len(dst, src, len) \ do { \ memcpy(__get_rel_str(dst), \ - __data_offsets.dst##_ptr_ ? : "(null)", len); \ + __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, len); \ __get_rel_str(dst)[len] = '\0'; \ } while (0) -- cgit v1.2.3 From c759e609030ca37e59866cbc849fdc611cc56292 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Fri, 23 Feb 2024 15:22:06 -0500 Subject: tracing: Remove __assign_str_len() Now that __assign_str() gets the length from the __string() (and __string_len()) macros, there's no reason to have a separate __assign_str_len() macro as __assign_str() can get the length of the string needed. Also remove __assign_rel_str() although it had no users anyway. Link: https://lore.kernel.org/linux-trace-kernel/20240223152206.0b650659@gandalf.local.home Cc: Jeff Layton Acked-by: Chuck Lever Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage6_event_callback.h | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 2bfd49713b42..0c0f50bcdc56 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -32,16 +32,13 @@ #undef __assign_str #define __assign_str(dst, src) \ - memcpy(__get_str(dst), __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, \ - __get_dynamic_array_len(dst)) - -#undef __assign_str_len -#define __assign_str_len(dst, src, len) \ do { \ - memcpy(__get_str(dst), \ - __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, len); \ - __get_str(dst)[len] = '\0'; \ - } while(0) + char *__str__ = __get_str(dst); \ + int __len__ = __get_dynamic_array_len(dst) - 1; \ + memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ + EVENT_NULL_STR, __len__); \ + __str__[__len__] = '\0'; \ + } while (0) #undef __assign_vstr #define __assign_vstr(dst, fmt, va) \ @@ -94,15 +91,12 @@ #undef __assign_rel_str #define __assign_rel_str(dst, src) \ - memcpy(__get_rel_str(dst), __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, \ - __get_rel_dynamic_array_len(dst)) - -#undef __assign_rel_str_len -#define __assign_rel_str_len(dst, src, len) \ do { \ - memcpy(__get_rel_str(dst), \ - __data_offsets.dst##_ptr_ ? : EVENT_NULL_STR, len); \ - __get_rel_str(dst)[len] = '\0'; \ + char *__str__ = __get_rel_str(dst); \ + int __len__ = __get_rel_dynamic_array_len(dst) - 1; \ + memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ + EVENT_NULL_STR, __len__); \ + __str__[__len__] = '\0'; \ } while (0) #undef __rel_bitmask -- cgit v1.2.3 From cf986e57d606849288eecf572800f2bd476fb1ab Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Fri, 23 Feb 2024 16:13:56 -0500 Subject: tracing: Add warning if string in __assign_str() does not match __string() In preparation to remove the second parameter of __assign_str(), make sure it is really a duplicate of __string() by adding a WARN_ON_ONCE(). Link: https://lore.kernel.org/linux-trace-kernel/20240223161356.63b72403@gandalf.local.home Cc: Mathieu Desnoyers Reviewed-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage6_event_callback.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include/trace') diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 0c0f50bcdc56..935608971899 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -35,6 +35,7 @@ do { \ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ + WARN_ON_ONCE((src) != __data_offsets.dst##_ptr_); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__); \ __str__[__len__] = '\0'; \ -- cgit v1.2.3 From 0bdfb68c845edd7e2dbe815266bb09641576e22f Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Fri, 23 Feb 2024 16:25:19 -0500 Subject: tracing: Remove second parameter to __assign_rel_str() The second parameter of __assign_rel_str() is no longer used. It can be removed. Note, the only real users of rel_string is user events. This code is just in the sample code for testing purposes. This makes __assign_rel_str() different than __assign_str() but that's fine. __assign_str() is used over 700 places and has a larger impact. That change will come later. Link: https://lore.kernel.org/linux-trace-kernel/20240223162519.2beb8112@gandalf.local.home Cc: Mathieu Desnoyers Reviewed-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage6_event_callback.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 935608971899..a0c15f67eabe 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -91,7 +91,7 @@ #define __rel_string_len(item, src, len) __rel_dynamic_array(char, item, -1) #undef __assign_rel_str -#define __assign_rel_str(dst, src) \ +#define __assign_rel_str(dst) \ do { \ char *__str__ = __get_rel_str(dst); \ int __len__ = __get_rel_dynamic_array_len(dst) - 1; \ -- cgit v1.2.3 From b1afefa62ca9d77c8b1d386c3512fb4f21cb7db1 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Tue, 12 Mar 2024 11:30:02 -0400 Subject: tracing: Use strcmp() in __assign_str() WARN_ON() check The WARN_ON() check in __assign_str() to catch where the source variable to the macro doesn't match the source variable to __string() gives an error in clang: >> include/trace/events/sunrpc.h:703:4: warning: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Wstring-compare] 670 | __assign_str(progname, "unknown"); That's because the __assign_str() macro has: WARN_ON_ONCE((src) != __data_offsets.dst##_ptr_); Where "src" is a string literal. Clang warns when comparing a string literal directly as it is undefined to what the value of the literal is. Since this is still to make sure the same string that goes to __string() is the same as __assign_str(), for string literals do a test for that and then use strcmp() in those cases Note that this depends on commit 51270d573a8d ("tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string") being applied, as this was what found that bug. Link: https://lore.kernel.org/linux-trace-kernel/20240312113002.00031668@gandalf.local.home Cc: Masami Hiramatsu Cc: Mathieu Desnoyers Cc: Nathan Chancellor Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202402292111.KIdExylU-lkp@intel.com/ Fixes: 433e1d88a3be ("tracing: Add warning if string in __assign_str() does not match __string()") Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage6_event_callback.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index a0c15f67eabe..83da83a0c14f 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -35,7 +35,9 @@ do { \ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ - WARN_ON_ONCE((src) != __data_offsets.dst##_ptr_); \ + WARN_ON_ONCE(__builtin_constant_p(src) ? \ + strcmp((src), __data_offsets.dst##_ptr_) : \ + (src) != __data_offsets.dst##_ptr_); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__); \ __str__[__len__] = '\0'; \ -- cgit v1.2.3 From 7604256cecef34a82333d9f78262d3180f4eb525 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Thu, 14 Mar 2024 23:27:54 -0400 Subject: tracing: Add __string_src() helper to help compilers not to get confused The __string() helper macro of the TRACE_EVENT() macro is used to determine how much of the ring buffer needs to be allocated to fit the given source string. Some trace events have a string that is dependent on another variable that could be NULL, and in those cases the string is passed in to be NULL. The __string() macro can handle being passed in a NULL pointer for which it will turn it into "(null)". It does that with: strlen((src) ? (const char *)(src) : "(null)") + 1 But if src itself has the same conditional type it can confuse the compiler. That is: __string(r ? dev(r)->name : NULL) Would turn into: strlen((r ? dev(r)->name : NULL) ? (r ? dev(r)->name : NULL) : "(null)" + 1 For which the compiler thinks that NULL is being passed to strlen() and gives this kind of warning: ./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull] 50 | strlen((src) ? (const char *)(src) : "(null)") + 1) Instead, create a static inline function that takes the src string and will return the string if it is not NULL and will return "(null)" if it is. This will then make the strlen() line: strlen(__string_src(src)) + 1 Where the compiler can see that strlen() will not end up with NULL and does not warn about it. Note that this depends on commit 51270d573a8d ("tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string") being applied, as passing the qdisc_dev() into __string_src() will give an error. Link: https://lore.kernel.org/all/ZfNmfCmgCs4Nc+EH@aschofie-mobl2/ Link: https://lore.kernel.org/linux-trace-kernel/20240314232754.345cea82@rorschach.local.home Cc: Masami Hiramatsu Cc: Mathieu Desnoyers Reported-by: Alison Schofield Signed-off-by: Steven Rostedt (Google) --- include/trace/stages/stage5_get_offsets.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h index e6b96608f452..c6a62dfb18ef 100644 --- a/include/trace/stages/stage5_get_offsets.h +++ b/include/trace/stages/stage5_get_offsets.h @@ -9,6 +9,16 @@ #undef __entry #define __entry entry +#ifndef __STAGE5_STRING_SRC_H +#define __STAGE5_STRING_SRC_H +static inline const char *__string_src(const char *str) +{ + if (!str) + return EVENT_NULL_STR; + return str; +} +#endif /* __STAGE5_STRING_SRC_H */ + /* * Fields should never declare an array: i.e. __field(int, arr[5]) * If they do, it will cause issues in parsing and possibly corrupt the @@ -47,7 +57,7 @@ #undef __string #define __string(item, src) __dynamic_array(char, item, \ - strlen((const char *)(src) ? : EVENT_NULL_STR) + 1) \ + strlen(__string_src(src)) + 1) \ __data_offsets->item##_ptr_ = src; #undef __string_len @@ -70,7 +80,7 @@ #undef __rel_string #define __rel_string(item, src) __rel_dynamic_array(char, item, \ - strlen((const char *)(src) ? : EVENT_NULL_STR) + 1) \ + strlen(__string_src(src)) + 1) \ __data_offsets->item##_ptr_ = src; #undef __rel_string_len -- cgit v1.2.3 From 24f5bb9f24ad80da6c6f83ba6124b5188c5384b2 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Tue, 19 Mar 2024 13:39:59 -0400 Subject: tracing: Just use strcmp() for testing __string() and __assign_str() match As __assign_str() no longer uses its "src" parameter, there's a check to make sure nothing depends on it being different than what was passed to __string(). It originally just compared the pointer passed to __string() with the pointer passed into __assign_str() via the "src" parameter. But there's a couple of outliers that just pass in a quoted string constant, where comparing the pointers is UB to the compiler, as the compiler is free to create multiple copies of the same string constant. Instead, just use strcmp(). It may slow down the trace event, but this will eventually be removed. Also, fix the issue of passing NULL to strcmp() by adding a WARN_ON() to make sure that both "src" and the pointer saved in __string() are either both NULL or have content, and then checking if "src" is not NULL before performing the strcmp(). Link: https://lore.kernel.org/all/CAHk-=wjxX16kWd=uxG5wzqt=aXoYDf1BgWOKk+qVmAO0zh7sjA@mail.gmail.com/ Fixes: b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON() check") Reported-by: Linus Torvalds Signed-off-by: Steven Rostedt (Google) Signed-off-by: Linus Torvalds --- include/trace/stages/stage6_event_callback.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'include/trace') diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 83da83a0c14f..3690e677263f 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -35,9 +35,8 @@ do { \ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ - WARN_ON_ONCE(__builtin_constant_p(src) ? \ - strcmp((src), __data_offsets.dst##_ptr_) : \ - (src) != __data_offsets.dst##_ptr_); \ + WARN_ON_ONCE(!(void *)(src) != !(void *)__data_offsets.dst##_ptr_); \ + WARN_ON_ONCE((src) && strcmp((src), __data_offsets.dst##_ptr_)); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__); \ __str__[__len__] = '\0'; \ -- cgit v1.2.3