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

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

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The gfp_flags when recorded in the trace require being converted from
their numbers to values. Various macros are used to help facilitate this,
but there's two sets of macros that need to keep track of the same GFP
flags to stay in sync.

Commit 60295b944ff68 ("tracing: gfp: Fix the GFP enum values shown for
user space tracing tools") added a TRACE_GFP_FLAGS macro that holds the
enum ___GFP_*_BIT defined bits, and creates the TRACE_DEFINE_ENUM()
wrapper around them.

The __def_gfpflag_names() macro creates the mapping of various flags or
multiple flags to give them human readable names via the __print_flags()
tracing helper macro.

As the TRACE_GFP_FLAGS is a subset of the __def_gfpflags_names(), it can
be used to cover the individual bit names, by redefining the internal
macro TRACE_GFP_EM():

  #undef TRACE_GFP_EM
  #define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),

This will remove the bits that are duplicate between the two macros. If a
new bit is created, only the TRACE_GFP_FLAGS needs to be updated and that
will also update the __def_gfpflags_names() macro.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Last version: https://lore.kernel.org/20250116214439.046082618@goodmis.org

  This was originally sent with a patch that fixed the output of gfp flags
  in trace events to show human readable flags and not hex numbers.

  This patch on the other hand is a clean up as the there's now two macros
  that define the bits to print. This makes the one macro use the other
  macro that is a subset of the first.

  Can someone in the memory management subsystem either give me an acked-by
  and I can take this through my tree, or you can just take this through
  the memory management tree. Either way works for me.

 include/trace/events/mmflags.h | 41 +++++++++-------------------------
 1 file changed, 10 insertions(+), 31 deletions(-)

diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index 72fbfe3caeaf..82371177ef79 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -78,6 +78,13 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
 
 #define gfpflag_string(flag) {(__force unsigned long)flag, #flag}
 
+/*
+ * For the values that match the bits, use the TRACE_GFP_FLAGS
+ * which will allow any updates to be included automatically.
+ */
+#undef TRACE_GFP_EM
+#define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
+
 #define __def_gfpflag_names			\
 	gfpflag_string(GFP_TRANSHUGE),		\
 	gfpflag_string(GFP_TRANSHUGE_LIGHT),	\
@@ -91,41 +98,13 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
 	gfpflag_string(GFP_NOIO),		\
 	gfpflag_string(GFP_NOWAIT),		\
 	gfpflag_string(GFP_DMA),		\
-	gfpflag_string(__GFP_HIGHMEM),		\
 	gfpflag_string(GFP_DMA32),		\
-	gfpflag_string(__GFP_HIGH),		\
-	gfpflag_string(__GFP_IO),		\
-	gfpflag_string(__GFP_FS),		\
-	gfpflag_string(__GFP_NOWARN),		\
-	gfpflag_string(__GFP_RETRY_MAYFAIL),	\
-	gfpflag_string(__GFP_NOFAIL),		\
-	gfpflag_string(__GFP_NORETRY),		\
-	gfpflag_string(__GFP_COMP),		\
-	gfpflag_string(__GFP_ZERO),		\
-	gfpflag_string(__GFP_NOMEMALLOC),	\
-	gfpflag_string(__GFP_MEMALLOC),		\
-	gfpflag_string(__GFP_HARDWALL),		\
-	gfpflag_string(__GFP_THISNODE),		\
-	gfpflag_string(__GFP_RECLAIMABLE),	\
-	gfpflag_string(__GFP_MOVABLE),		\
-	gfpflag_string(__GFP_ACCOUNT),		\
-	gfpflag_string(__GFP_WRITE),		\
 	gfpflag_string(__GFP_RECLAIM),		\
-	gfpflag_string(__GFP_DIRECT_RECLAIM),	\
-	gfpflag_string(__GFP_KSWAPD_RECLAIM),	\
-	gfpflag_string(__GFP_ZEROTAGS)
-
-#ifdef CONFIG_KASAN_HW_TAGS
-#define __def_gfpflag_names_kasan ,			\
-	gfpflag_string(__GFP_SKIP_ZERO),		\
-	gfpflag_string(__GFP_SKIP_KASAN)
-#else
-#define __def_gfpflag_names_kasan
-#endif
+	TRACE_GFP_FLAGS				\
+	{ 0, "none" }
 
 #define show_gfp_flags(flags)						\
-	(flags) ? __print_flags(flags, "|",				\
-	__def_gfpflag_names __def_gfpflag_names_kasan			\
+	(flags) ? __print_flags(flags, "|", __def_gfpflag_names		\
 	) : "none"
 
 #ifdef CONFIG_MMU
-- 
2.47.2

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

[Steven Rostedt]

On Tue, 25 Feb 2025 13:56:11 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> Last version: https://lore.kernel.org/20250116214439.046082618@goodmis.org
> 
>   This was originally sent with a patch that fixed the output of gfp flags
>   in trace events to show human readable flags and not hex numbers.
> 
>   This patch on the other hand is a clean up as the there's now two macros
>   that define the bits to print. This makes the one macro use the other
>   macro that is a subset of the first.
> 
>   Can someone in the memory management subsystem either give me an acked-by
>   and I can take this through my tree, or you can just take this through
>   the memory management tree. Either way works for me.

Interesting, I even ran a before and after of this patch by doing the following:

 # trace-cmd start -e dma -e vmscan -e percpu -e kmem
 [ wait a few minutes ]

 # trace-cmd show |grep gfp > ~/gfp-before

 [ apply patch, compile, install, reboot ]

 # trace-cmd start -e dma -e vmscan -e percpu -e kmem
 [ wait a few minutes ]

 # trace-cmd show |grep gfp > ~/gfp-after

 # perl -e 'while (<>) { if (/gfp_flags=(\S+)/) { print "$1\n"; } }' < gfp-before  | sort -u  > /tmp/before.sort
 # perl -e 'while (<>) { if (/gfp_flags=(\S+)/) { print "$1\n"; } }' < gfp-after  | sort -u  > /tmp/after.sort

 # diff -u /tmp/before.sort /tmp/after.sort 
--- /tmp/before.sort	2025-02-25 14:41:49.799742048 -0500
+++ /tmp/after.sort	2025-02-25 14:41:41.247636893 -0500
@@ -4,38 +4,39 @@
 GFP_ATOMIC|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP
 GFP_ATOMIC|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC
 GFP_ATOMIC|__GFP_NOWARN|__GFP_NORETRY|__GFP_NOMEMALLOC
-GFP_ATOMIC|__GFP_ZERO|0x2000000
+GFP_ATOMIC|__GFP_ZERO|__GFP_COMP|__GFP_NO_OBJ_EXT
 GFP_HIGHUSER|__GFP_ACCOUNT
 GFP_HIGHUSER_MOVABLE|__GFP_COMP
-GFP_HIGHUSER_MOVABLE|__GFP_COMP|__GFP_WRITE
-GFP_HIGHUSER_MOVABLE|__GFP_COMP|__GFP_ZERO
+GFP_HIGHUSER_MOVABLE|__GFP_WRITE|__GFP_COMP
+GFP_HIGHUSER_MOVABLE|__GFP_ZERO|__GFP_COMP
 __GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC
-__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC|__GFP_RECLAIMABLE
-__GFP_IO|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC|__GFP_RECLAIMABLE
 GFP_KERNEL
 GFP_KERNEL_ACCOUNT
-GFP_KERNEL_ACCOUNT|__GFP_COMP|__GFP_ZERO
+GFP_KERNEL_ACCOUNT|__GFP_NOWARN|__GFP_NOMEMALLOC
 GFP_KERNEL_ACCOUNT|__GFP_ZERO
-GFP_KERNEL|__GFP_COMP|__GFP_ZERO|0x2000000
+GFP_KERNEL_ACCOUNT|__GFP_ZERO|__GFP_COMP
 GFP_KERNEL|__GFP_NOWARN|__GFP_NOMEMALLOC
-GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP
 GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY|__GFP_NOMEMALLOC
 GFP_KERNEL|__GFP_ZERO
+GFP_KERNEL|__GFP_ZERO|__GFP_COMP|__GFP_NO_OBJ_EXT
+GFP_KERNEL|__GFP_ZERO|__GFP_NO_OBJ_EXT
 GFP_NOFS
-GFP_NOFS|__GFP_COMP|__GFP_ZERO|0x2000000
-GFP_NOFS|__GFP_NOFAIL|__GFP_COMP|__GFP_HARDWALL|__GFP_MOVABLE
-GFP_NOFS|__GFP_NOFAIL|__GFP_ZERO
-GFP_NOFS|__GFP_NOFAIL|__GFP_ZERO|__GFP_ACCOUNT
-GFP_NOFS|__GFP_NOFAIL|__GFP_ZERO|__GFP_HARDWALL|__GFP_MOVABLE|__GFP_ACCOUNT
+GFP_NOFS|__GFP_MOVABLE|__GFP_NOFAIL|__GFP_COMP|__GFP_HARDWALL
+GFP_NOFS|__GFP_MOVABLE|__GFP_ZERO|__GFP_NOFAIL|__GFP_HARDWALL|__GFP_ACCOUNT
+GFP_NOFS|__GFP_NOFAIL
 GFP_NOFS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP
-GFP_NOFS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_RECLAIMABLE
 GFP_NOFS|__GFP_NOWARN|__GFP_NORETRY|__GFP_NOMEMALLOC
+GFP_NOFS|__GFP_RECLAIMABLE|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP
 GFP_NOFS|__GFP_ZERO
-GFP_NOFS|__GFP_ZERO|0x2000000
+GFP_NOFS|__GFP_ZERO|__GFP_NOFAIL
+GFP_NOFS|__GFP_ZERO|__GFP_NOFAIL|__GFP_ACCOUNT
+GFP_NOFS|__GFP_ZERO|__GFP_NO_OBJ_EXT
 GFP_NOWAIT
 GFP_NOWAIT|__GFP_ACCOUNT
-GFP_NOWAIT|__GFP_IO|__GFP_FS|__GFP_NORETRY|__GFP_COMP
+GFP_NOWAIT|__GFP_COMP
 GFP_NOWAIT|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC
-GFP_NOWAIT|__GFP_NORETRY|__GFP_COMP|__GFP_RECLAIMABLE
 GFP_NOWAIT|__GFP_NORETRY|__GFP_NOMEMALLOC
-GFP_NOWAIT|__GFP_ZERO|0x2000000
+GFP_NOWAIT|__GFP_RECLAIMABLE|__GFP_NORETRY|__GFP_COMP
+GFP_NOWAIT|__GFP_ZERO|__GFP_ACCOUNT
+GFP_NOWAIT|__GFP_ZERO|__GFP_NO_OBJ_EXT
+__GFP_RECLAIMABLE|__GFP_IO|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC

Notice that the old way has:

-GFP_NOFS|__GFP_COMP|__GFP_ZERO|0x2000000

And I looked at what that 0x2000000 is, and for my current config, it is:

  __GFP_NO_OBJ_EXT

Which was completely missing from the old way, and this patch actually
picks it up!

That's because the TRACE_GFP_FLAGS has it, but the __def_gfpflag_names
macro was missing it. Again, it's better to remove having to maintain two
lists instead of just one.

-- Steve

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

[Steven Rostedt]

Since I hear only crickets on this. I'm going to apply it and push it to
linux-next and see if anyone notices. It only affects the output of the
memory trace events.

-- Steve


On Tue, 25 Feb 2025 13:56:11 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> The gfp_flags when recorded in the trace require being converted from
> their numbers to values. Various macros are used to help facilitate this,
> but there's two sets of macros that need to keep track of the same GFP
> flags to stay in sync.
> 
> Commit 60295b944ff68 ("tracing: gfp: Fix the GFP enum values shown for
> user space tracing tools") added a TRACE_GFP_FLAGS macro that holds the
> enum ___GFP_*_BIT defined bits, and creates the TRACE_DEFINE_ENUM()
> wrapper around them.
> 
> The __def_gfpflag_names() macro creates the mapping of various flags or
> multiple flags to give them human readable names via the __print_flags()
> tracing helper macro.
> 
> As the TRACE_GFP_FLAGS is a subset of the __def_gfpflags_names(), it can
> be used to cover the individual bit names, by redefining the internal
> macro TRACE_GFP_EM():
> 
>   #undef TRACE_GFP_EM
>   #define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
> 
> This will remove the bits that are duplicate between the two macros. If a
> new bit is created, only the TRACE_GFP_FLAGS needs to be updated and that
> will also update the __def_gfpflags_names() macro.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

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

[Petr Mladek]

On Tue 2025-02-25 13:56:11, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> The gfp_flags when recorded in the trace require being converted from
> their numbers to values. Various macros are used to help facilitate this,
> but there's two sets of macros that need to keep track of the same GFP
> flags to stay in sync.
> 
> Commit 60295b944ff68 ("tracing: gfp: Fix the GFP enum values shown for
> user space tracing tools") added a TRACE_GFP_FLAGS macro that holds the
> enum ___GFP_*_BIT defined bits, and creates the TRACE_DEFINE_ENUM()
> wrapper around them.
> 
> The __def_gfpflag_names() macro creates the mapping of various flags or
> multiple flags to give them human readable names via the __print_flags()
> tracing helper macro.
> 
> As the TRACE_GFP_FLAGS is a subset of the __def_gfpflags_names(), it can
> be used to cover the individual bit names, by redefining the internal
> macro TRACE_GFP_EM():
> 
>   #undef TRACE_GFP_EM
>   #define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
> 
> This will remove the bits that are duplicate between the two macros. If a
> new bit is created, only the TRACE_GFP_FLAGS needs to be updated and that
> will also update the __def_gfpflags_names() macro.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Last version: https://lore.kernel.org/20250116214439.046082618@goodmis.org
> 
>   This was originally sent with a patch that fixed the output of gfp flags
>   in trace events to show human readable flags and not hex numbers.
> 
>   This patch on the other hand is a clean up as the there's now two macros
>   that define the bits to print. This makes the one macro use the other
>   macro that is a subset of the first.
> 
>   Can someone in the memory management subsystem either give me an acked-by
>   and I can take this through my tree, or you can just take this through
>   the memory management tree. Either way works for me.
> 
>  include/trace/events/mmflags.h | 41 +++++++++-------------------------
>  1 file changed, 10 insertions(+), 31 deletions(-)
> 
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index 72fbfe3caeaf..82371177ef79 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -78,6 +78,13 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
>  
>  #define gfpflag_string(flag) {(__force unsigned long)flag, #flag}
>  
> +/*
> + * For the values that match the bits, use the TRACE_GFP_FLAGS
> + * which will allow any updates to be included automatically.
> + */
> +#undef TRACE_GFP_EM
> +#define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
> +
>  #define __def_gfpflag_names			\
>  	gfpflag_string(GFP_TRANSHUGE),		\
>  	gfpflag_string(GFP_TRANSHUGE_LIGHT),	\
> @@ -91,41 +98,13 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
>  	gfpflag_string(GFP_NOIO),		\
>  	gfpflag_string(GFP_NOWAIT),		\
>  	gfpflag_string(GFP_DMA),		\
> -	gfpflag_string(__GFP_HIGHMEM),		\
>  	gfpflag_string(GFP_DMA32),		\
> -	gfpflag_string(__GFP_HIGH),		\
> -	gfpflag_string(__GFP_IO),		\
> -	gfpflag_string(__GFP_FS),		\
> -	gfpflag_string(__GFP_NOWARN),		\
> -	gfpflag_string(__GFP_RETRY_MAYFAIL),	\
> -	gfpflag_string(__GFP_NOFAIL),		\
> -	gfpflag_string(__GFP_NORETRY),		\
> -	gfpflag_string(__GFP_COMP),		\
> -	gfpflag_string(__GFP_ZERO),		\
> -	gfpflag_string(__GFP_NOMEMALLOC),	\
> -	gfpflag_string(__GFP_MEMALLOC),		\
> -	gfpflag_string(__GFP_HARDWALL),		\
> -	gfpflag_string(__GFP_THISNODE),		\
> -	gfpflag_string(__GFP_RECLAIMABLE),	\
> -	gfpflag_string(__GFP_MOVABLE),		\
> -	gfpflag_string(__GFP_ACCOUNT),		\
> -	gfpflag_string(__GFP_WRITE),		\
>  	gfpflag_string(__GFP_RECLAIM),		\
> -	gfpflag_string(__GFP_DIRECT_RECLAIM),	\
> -	gfpflag_string(__GFP_KSWAPD_RECLAIM),	\
> -	gfpflag_string(__GFP_ZEROTAGS)
> -
> -#ifdef CONFIG_KASAN_HW_TAGS
> -#define __def_gfpflag_names_kasan ,			\
> -	gfpflag_string(__GFP_SKIP_ZERO),		\
> -	gfpflag_string(__GFP_SKIP_KASAN)
> -#else
> -#define __def_gfpflag_names_kasan
> -#endif
> +	TRACE_GFP_FLAGS				\
> +	{ 0, "none" }

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.

>  
>  #define show_gfp_flags(flags)						\
> -	(flags) ? __print_flags(flags, "|",				\
> -	__def_gfpflag_names __def_gfpflag_names_kasan			\
> +	(flags) ? __print_flags(flags, "|", __def_gfpflag_names		\
>  	) : "none"
>  
>  #ifdef CONFIG_MMU

Best Regards,
Petr

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

[Steven Rostedt]

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

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

[Steven Rostedt]

On Thu, 13 Mar 2025 12:53:13 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > --- 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?

OK, I think this is too hacky, and it only affects tracing if there's a
flag not defined (which never happened so I didn't see this issue).

I'll just go with your approach.

You want to send a formal patch?

-- Steve

[PATCH] tracing: gfp: vsprintf: Do not print "none" when using %pGg printf format

[Petr Mladek]

The commit ca29a0bf122145 ("tracing: gfp: Remove duplication of recording
GFP flags") caused the following regression in printf_test selftest:

[   46.208199] test_printf: kvasprintf(..., "%pGg", ...) returned 'none|0xfc000000', expected '0xfc000000'
[   46.208209] test_printf: kvasprintf(..., "%pGg", ...) returned '__GFP_HIGH|none|0xfc000000', expected '__GFP_HIGH|0xfc000000'

The problem is the new '{ 0, "none" }' entry in __def_gfpflag_names macro
and the following code:

char *format_flags(char *buf, char *end, unsigned long flags,
					const struct trace_print_flags *names)
{
[...]
		if ((flags & mask) != mask)
			continue;
[...]
}

The purpose of the code is to print the name of a mask instead of bits,
for example, printk "GFP_ZONEMASK", instead of
"__GFP_DMA|__GFP_HIGHMEM|__GFP_DMA32|__GFP_MOVABLE".

Unfortunately, the mask "0" pass this check and "none" is always
printed.

A solution would be to move TRACE_GFP_FLAGS up so that it is not
the last entry. But it breaks the rule that named masks must
be defined before names of single bytes. Otherwise, it would
print the names of the bytes instead of the mask.

Instead, replace '{ 0, "none" }' with '{ 0, NULL }'. It works because
__def_gfpflag_names defines a standalone array and this is the standard
trailing entry. The code processing these arrays always ends the cycle
when flag->name == NULL.

Fixes: ca29a0bf122145 ("tracing: gfp: Remove duplication of recording GFP flags")
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/trace/events/mmflags.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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		\
-- 
2.48.1