Re: [RESEND][PATCH] tracing: gfp: Remove duplication of recording GFP flags

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 13 Mar 2025 16:26:22 +0100
Petr Mladek <pmladek@suse.com> wrote:

> This causes regression in the printf selftest:
> 
> # modprobe test_printf
> modprobe: ERROR: could not insert 'test_printf': Invalid argument
> 
> # dmesg | tail 
> [   46.206779] test_printf: vsnprintf(buf, 256, "%pGg", ...) returned 15, expected 10
> [   46.208192] test_printf: vsnprintf(buf, 3, "%pGg", ...) returned 15, expected 10
> [   46.208196] test_printf: vsnprintf(buf, 0, "%pGg", ...) returned 15, expected 10
> [   46.208199] test_printf: kvasprintf(..., "%pGg", ...) returned 'none|0xfc000000', expected '0xfc000000'
> [   46.208202] test_printf: vsnprintf(buf, 256, "%pGg", ...) returned 26, expected 21
> [   46.208204] test_printf: vsnprintf(buf, 17, "%pGg", ...) returned 26, expected 21
> [   46.208206] test_printf: vsnprintf(buf, 0, "%pGg", ...) returned 26, expected 21
> [   46.208209] test_printf: kvasprintf(..., "%pGg", ...) returned '__GFP_HIGH|none|0xfc000000', expected '__GFP_HIGH|0xfc000000'
> [   46.208865] test_printf: failed 8 out of 448 tests
> 
>     => vprintf() started printing the "none|" string.  
> 
> It seems to me that "{ 0, "none" }" was added as an "innocent" entry
> to avoid the trailing "," generated by TRACE_GFP_FLAGS. So, it is
> not really needed.
> 
> In fact, I think that it probably causes similar regression in the
> trace output because the logic in trace_print_flags_seq()
> seems to be the same as in format_flags() in lib/vsprintf.c.
> 
> The following worked for me:
> 
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index 82371177ef79..15aae955a10b 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -101,7 +101,7 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
>  	gfpflag_string(GFP_DMA32),		\
>  	gfpflag_string(__GFP_RECLAIM),		\
>  	TRACE_GFP_FLAGS				\
> -	{ 0, "none" }
> +	{ 0, NULL }
>  
>  #define show_gfp_flags(flags)						\
>  	(flags) ? __print_flags(flags, "|", __def_gfpflag_names		\
> 
> It seems to be safe because the callers end up the cycle when .name == NULL.
> 
> I think that it actually allows to remove similar trailing {} but I am not sure
> if we want it.

Hmm, I could get rid of that last one with this patch. What do you think?

diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index 82371177ef79..74af49c33d14 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -37,26 +37,26 @@
 	TRACE_GFP_EM(HARDWALL)			\
 	TRACE_GFP_EM(THISNODE)			\
 	TRACE_GFP_EM(ACCOUNT)			\
-	TRACE_GFP_EM(ZEROTAGS)
+	TRACE_GFP_EME(ZEROTAGS)
 
 #ifdef CONFIG_KASAN_HW_TAGS
 # define TRACE_GFP_FLAGS_KASAN			\
-	TRACE_GFP_EM(SKIP_ZERO)			\
-	TRACE_GFP_EM(SKIP_KASAN)
+	TRACE_COMMA TRACE_GFP_EM(SKIP_ZERO)	\
+	TRACE_GFP_EME(SKIP_KASAN)
 #else
 # define TRACE_GFP_FLAGS_KASAN
 #endif
 
 #ifdef CONFIG_LOCKDEP
 # define TRACE_GFP_FLAGS_LOCKDEP		\
-	TRACE_GFP_EM(NOLOCKDEP)
+	TRACE_COMMA TRACE_GFP_EME(NOLOCKDEP)
 #else
 # define TRACE_GFP_FLAGS_LOCKDEP
 #endif
 
 #ifdef CONFIG_SLAB_OBJ_EXT
 # define TRACE_GFP_FLAGS_SLAB			\
-	TRACE_GFP_EM(NO_OBJ_EXT)
+	TRACE_COMMA TRACE_GFP_EME(NO_OBJ_EXT)
 #else
 # define TRACE_GFP_FLAGS_SLAB
 #endif
@@ -69,6 +69,10 @@
 
 #undef TRACE_GFP_EM
 #define TRACE_GFP_EM(a) TRACE_DEFINE_ENUM(___GFP_##a##_BIT);
+#undef TRACE_GFP_EME
+#define TRACE_GFP_EME(a) TRACE_DEFINE_ENUM(___GFP_##a##_BIT);
+#undef TRACE_COMMA
+#define TRACE_COMMA
 
 TRACE_GFP_FLAGS
 
@@ -84,6 +88,10 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
  */
 #undef TRACE_GFP_EM
 #define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
+#undef TRACE_GFP_EME
+#define TRACE_GFP_EME(a) gfpflag_string(__GFP_##a)
+#undef TRACE_COMMA
+#define TRACE_COMMA ,
 
 #define __def_gfpflag_names			\
 	gfpflag_string(GFP_TRANSHUGE),		\
@@ -100,8 +108,7 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
 	gfpflag_string(GFP_DMA),		\
 	gfpflag_string(GFP_DMA32),		\
 	gfpflag_string(__GFP_RECLAIM),		\
-	TRACE_GFP_FLAGS				\
-	{ 0, "none" }
+	TRACE_GFP_FLAGS
 
 #define show_gfp_flags(flags)						\
 	(flags) ? __print_flags(flags, "|", __def_gfpflag_names		\

-- Steve