| Message ID | 20230908163929.2c25f3dc@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fc52a64416b010c8324e2cb50070faae868521c1 |
| Headers | show |
| Series | [RESEND] tracing/synthetic: Fix order of struct trace_dynamic_info | expand |
On Fri, 8 Sep 2023 16:39:29 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > To make handling BIG and LITTLE endian better the offset/len of dynamic > fields of the synthetic events was changed into a structure of: > > struct trace_dynamic_info { > #ifdef CONFIG_CPU_BIG_ENDIAN > u16 offset; > u16 len; > #else > u16 len; > u16 offset; > #endif > }; > > to replace the manual changes of: > > data_offset = offset & 0xffff; > data_offest = len << 16; > > But if you look closely, the above is: > > <len> << 16 | offset > > Which in little endian would be in memory: > > offset_lo offset_hi len_lo len_hi > > and in big endian: > > len_hi len_lo offset_hi offset_lo > > Which if broken into a structure would be: > > struct trace_dynamic_info { > #ifdef CONFIG_CPU_BIG_ENDIAN > u16 len; > u16 offset; > #else > u16 offset; > u16 len; > #endif > }; > > Which is the opposite of what was defined. > > Fix this and just to be safe also add "__packed". > > Link: https://lore.kernel.org/all/20230908154417.5172e343@gandalf.local.home/ Good catch! I'm not sure why this worked. Maybe we don't have any testcase for this feature? Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you, > > Cc: stable@vger.kernel.org > Fixes: ddeea494a16f3 ("tracing/synthetic: Use union instead of casts") > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > > [ Resending to the correct mailing list this time :-p ] > > include/linux/trace_events.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h > index 12f875e9e69a..21ae37e49319 100644 > --- a/include/linux/trace_events.h > +++ b/include/linux/trace_events.h > @@ -62,13 +62,13 @@ void trace_event_printf(struct trace_iterator *iter, const char *fmt, ...); > /* Used to find the offset and length of dynamic fields in trace events */ > struct trace_dynamic_info { > #ifdef CONFIG_CPU_BIG_ENDIAN > - u16 offset; > u16 len; > + u16 offset; > #else > - u16 len; > u16 offset; > + u16 len; > #endif > -}; > +} __packed; > > /* > * The trace entry - the most basic unit of tracing. This is what > -- > 2.40.1 >
Hi Steven, Steven Rostedt <rostedt@goodmis.org> writes: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > To make handling BIG and LITTLE endian better the offset/len of dynamic > fields of the synthetic events was changed into a structure of: > > struct trace_dynamic_info { > #ifdef CONFIG_CPU_BIG_ENDIAN > u16 offset; > u16 len; > #else > u16 len; > u16 offset; > #endif > }; > > to replace the manual changes of: > > data_offset = offset & 0xffff; > data_offest = len << 16; > > But if you look closely, the above is: > > <len> << 16 | offset > > Which in little endian would be in memory: > > offset_lo offset_hi len_lo len_hi > > and in big endian: > > len_hi len_lo offset_hi offset_lo > > Which if broken into a structure would be: > > struct trace_dynamic_info { > #ifdef CONFIG_CPU_BIG_ENDIAN > u16 len; > u16 offset; > #else > u16 offset; > u16 len; > #endif > }; > > Which is the opposite of what was defined. > > Fix this and just to be safe also add "__packed". > > Link: https://lore.kernel.org/all/20230908154417.5172e343@gandalf.local.home/ > > Cc: stable@vger.kernel.org > Fixes: ddeea494a16f3 ("tracing/synthetic: Use union instead of casts") > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > > [ Resending to the correct mailing list this time :-p ] > > include/linux/trace_events.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h > index 12f875e9e69a..21ae37e49319 100644 > --- a/include/linux/trace_events.h > +++ b/include/linux/trace_events.h > @@ -62,13 +62,13 @@ void trace_event_printf(struct trace_iterator *iter, const char *fmt, ...); > /* Used to find the offset and length of dynamic fields in trace events */ > struct trace_dynamic_info { > #ifdef CONFIG_CPU_BIG_ENDIAN > - u16 offset; > u16 len; > + u16 offset; > #else > - u16 len; > u16 offset; > + u16 len; > #endif > -}; > +} __packed; > > /* > * The trace entry - the most basic unit of tracing. This is what This issue was also present on BE, but as you noted "covered" by the broken test case. With this patch everything works as expected. So: Tested-by: Sven Schnelle <svens@linux.ibm.com>
On Mon, 11 Sep 2023 09:26:12 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > Fix this and just to be safe also add "__packed". > > > > Link: https://lore.kernel.org/all/20230908154417.5172e343@gandalf.local.home/ > > Good catch! I'm not sure why this worked. Maybe we don't have any testcase > for this feature? We have a test case, it was broken by a change in the README :-p I noticed issues with it, and then looked at the tests, fixed the test and it failed. Before that, it was just reporting "unsupported", which is why I included that fix with this queue. https://lore.kernel.org/linux-trace-kernel/20230614091046.2178539-1-naveen@kernel.org/ > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thanks! -- Steve
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index 12f875e9e69a..21ae37e49319 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -62,13 +62,13 @@ void trace_event_printf(struct trace_iterator *iter, const char *fmt, ...); /* Used to find the offset and length of dynamic fields in trace events */ struct trace_dynamic_info { #ifdef CONFIG_CPU_BIG_ENDIAN - u16 offset; u16 len; + u16 offset; #else - u16 len; u16 offset; + u16 len; #endif -}; +} __packed; /* * The trace entry - the most basic unit of tracing. This is what