[PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

Tracing tools like perf and trace-cmd read the /sys/kernel/tracing/events/*/*/format
files to know how to parse the data and also how to print it. For the
"print fmt" portion of that file, if anything uses an enum that is not
exported to the tracing system, user space will not be able to parse it.

The GFP flags use to be defines, and defines get translated in the print
fmt sections. But now they are converted to use enums, which is not.

The mm_page_alloc trace event format use to have:

  print fmt: "page=%p pfn=0x%lx order=%d migratetype=%d gfp_flags=%s",
    REC->pfn != -1UL ? (((struct page *)vmemmap_base) + (REC->pfn)) : ((void
    *)0), REC->pfn != -1UL ? REC->pfn : 0, REC->order, REC->migratetype,
    (REC->gfp_flags) ? __print_flags(REC->gfp_flags, "|", {( unsigned
    long)(((((((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u) |
    (( gfp_t)0x100000u)) | (( gfp_t)0x02u)) | (( gfp_t)0x08u) | (( gfp_t)0)) |
    (( gfp_t)0x40000u) | (( gfp_t)0x80000u) | (( gfp_t)0x2000u)) & ~((
    gfp_t)(0x400u|0x800u))) | (( gfp_t)0x400u)), "GFP_TRANSHUGE"}, {( unsigned
    long)((((((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u) |
    (( gfp_t)0x100000u)) | (( gfp_t)0x02u)) | (( gfp_t)0x08u) | (( gfp_t)0)) ...

Where the GFP values are shown and not their names. But after the GFP
flags were converted to use enums, it has:

  print fmt: "page=%p pfn=0x%lx order=%d migratetype=%d gfp_flags=%s",
    REC->pfn != -1UL ? (vmemmap + (REC->pfn)) : ((void *)0), REC->pfn != -1UL
    ? REC->pfn : 0, REC->order, REC->migratetype, (REC->gfp_flags) ?
    __print_flags(REC->gfp_flags, "|", {( unsigned long)((((((((
    gfp_t)(((((1UL))) << (___GFP_DIRECT_RECLAIM_BIT))|((((1UL))) <<
    (___GFP_KSWAPD_RECLAIM_BIT)))) | (( gfp_t)((((1UL))) << (___GFP_IO_BIT)))
    | (( gfp_t)((((1UL))) << (___GFP_FS_BIT))) | (( gfp_t)((((1UL))) <<
    (___GFP_HARDWALL_BIT)))) | (( gfp_t)((((1UL))) << (___GFP_HIGHMEM_BIT))))
    | (( gfp_t)((((1UL))) << (___GFP_MOVABLE_BIT))) | (( gfp_t)0)) | ((
    gfp_t)((((1UL))) << (___GFP_COMP_BIT))) ...

Where the enums names like ___GFP_KSWAPD_RECLAIM_BIT are shown and not their
values. User space has no way to convert these names to their values and
the output will fail to parse. What is shown is now:

  mm_page_alloc:  page=0xffffffff981685f3 pfn=0x1d1ac1 order=0 migratetype=1 gfp_flags=0x140cca

The TRACE_DEFINE_ENUM() macro was created to handle enums in the print fmt
files. This causes them to be replaced at boot up with the numbers, so
that user space tooling can parse it. By using this macro, the output is
back to the human readable:

  mm_page_alloc: page=0xffffffff981685f3 pfn=0x122233 order=0 migratetype=1 gfp_flags=GFP_HIGHUSER_MOVABLE|__GFP_COMP

Cc: stable@vger.kernel.org
Reported-by: Michael Petlan <mpetlan@redhat.com>
Closes: https://lore.kernel.org/all/87be5f7c-1a0-dad-daa0-54e342efaea7@redhat.com/
Fixes: 772dd0342727c ("mm: enumerate all gfp flags")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/linux/gfp_types.h      | 47 ++++++++++++++++++++++++++++++++++
 include/trace/events/kmem.h    |  2 ++
 include/trace/events/mmflags.h |  2 ++
 3 files changed, 51 insertions(+)

diff --git a/include/linux/gfp_types.h b/include/linux/gfp_types.h
index 65db9349f905..57efa0310900 100644
--- a/include/linux/gfp_types.h
+++ b/include/linux/gfp_types.h
@@ -104,6 +104,53 @@ enum {
 #define ___GFP_NO_OBJ_EXT       0
 #endif
 
+/* Need to have GFP flags convert to numbers in trace event format files */
+#define TRACE_DEFINE_GFP_FLAGS_GENERAL			\
+	TRACE_DEFINE_ENUM(___GFP_DMA_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_HIGHMEM_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_DMA32_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_MOVABLE_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_RECLAIMABLE_BIT);	\
+	TRACE_DEFINE_ENUM(___GFP_HIGH_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_IO_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_FS_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_ZERO_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_UNUSED_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_DIRECT_RECLAIM_BIT);	\
+	TRACE_DEFINE_ENUM(___GFP_KSWAPD_RECLAIM_BIT);	\
+	TRACE_DEFINE_ENUM(___GFP_WRITE_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_NOWARN_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_RETRY_MAYFAIL_BIT);	\
+	TRACE_DEFINE_ENUM(___GFP_NOFAIL_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_NORETRY_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_MEMALLOC_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_COMP_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_NOMEMALLOC_BIT);	\
+	TRACE_DEFINE_ENUM(___GFP_HARDWALL_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_THISNODE_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_ACCOUNT_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_ZEROTAGS_BIT);		\
+	TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
+
+#ifdef CONFIG_KASAN_HW_TAGS
+# define TRACE_DEFINE_GFP_FLAGS_KASAN			\
+	TRACE_DEFINE_ENUM(___GFP_SKIP_ZERO_BIT);	\
+	TRACE_DEFINE_ENUM(___GFP_SKIP_KASAN_BIT);
+#else
+# define TRACE_DEFINE_GFP_FLAGS_KASAN
+#endif
+#ifdef CONFIG_LOCKDEP
+# define TRACE_DEFINE_GFP_FLAGS_LOCKDEP			\
+	TRACE_DEFINE_ENUM(___GFP_NOLOCKDEP_BIT);
+#else
+# define TRACE_DEFINE_GFP_FLAGS_LOCKDEP
+#endif
+
+#define TRACE_DEFINE_GFP_FLAGS			\
+	TRACE_DEFINE_GFP_FLAGS_GENERAL		\
+	TRACE_DEFINE_GFP_FLAGS_KASAN		\
+	TRACE_DEFINE_GFP_FLAGS_LOCKDEP
+
 /*
  * Physical address zone modifiers (see linux/mmzone.h - low four bits)
  *
diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
index b37eb0a7060f..e32098c0f187 100644
--- a/include/trace/events/kmem.h
+++ b/include/trace/events/kmem.h
@@ -9,6 +9,8 @@
 #include <linux/tracepoint.h>
 #include <trace/events/mmflags.h>
 
+TRACE_DEFINE_GFP_FLAGS
+
 TRACE_EVENT(kmem_cache_alloc,
 
 	TP_PROTO(unsigned long call_site,
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index bb8a59c6caa2..522bbe3a5fe1 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -15,6 +15,8 @@
 
 #define gfpflag_string(flag) {(__force unsigned long)flag, #flag}
 
+TRACE_DEFINE_GFP_FLAGS
+
 #define __def_gfpflag_names			\
 	gfpflag_string(GFP_TRANSHUGE),		\
 	gfpflag_string(GFP_TRANSHUGE_LIGHT),	\
-- 
2.45.2

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Steven Rostedt]

On Thu, 16 Jan 2025 13:23:59 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> +#define TRACE_DEFINE_GFP_FLAGS_GENERAL			\
> +	TRACE_DEFINE_ENUM(___GFP_DMA_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_HIGHMEM_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_DMA32_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_MOVABLE_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_RECLAIMABLE_BIT);	\
> +	TRACE_DEFINE_ENUM(___GFP_HIGH_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_IO_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_FS_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_ZERO_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_UNUSED_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_DIRECT_RECLAIM_BIT);	\
> +	TRACE_DEFINE_ENUM(___GFP_KSWAPD_RECLAIM_BIT);	\
> +	TRACE_DEFINE_ENUM(___GFP_WRITE_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_NOWARN_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_RETRY_MAYFAIL_BIT);	\
> +	TRACE_DEFINE_ENUM(___GFP_NOFAIL_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_NORETRY_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_MEMALLOC_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_COMP_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_NOMEMALLOC_BIT);	\
> +	TRACE_DEFINE_ENUM(___GFP_HARDWALL_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_THISNODE_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_ACCOUNT_BIT);		\
> +	TRACE_DEFINE_ENUM(___GFP_ZEROTAGS_BIT);		\

> +	TRACE_DEFINE_ENUM(___GFP_LAST_BIT);

Hmm, I don't think I need to include the LAST_BIT.

> +
> +#ifdef CONFIG_KASAN_HW_TAGS
> +# define TRACE_DEFINE_GFP_FLAGS_KASAN			\
> +	TRACE_DEFINE_ENUM(___GFP_SKIP_ZERO_BIT);	\
> +	TRACE_DEFINE_ENUM(___GFP_SKIP_KASAN_BIT);
> +#else
> +# define TRACE_DEFINE_GFP_FLAGS_KASAN
> +#endif
> +#ifdef CONFIG_LOCKDEP
> +# define TRACE_DEFINE_GFP_FLAGS_LOCKDEP			\
> +	TRACE_DEFINE_ENUM(___GFP_NOLOCKDEP_BIT);
> +#else
> +# define TRACE_DEFINE_GFP_FLAGS_LOCKDEP
> +#endif

I also missed adding CONFIG_SLAB_OBJ_EXT.

I'll send a v2 shortly.

-- Steve

> +
> +#define TRACE_DEFINE_GFP_FLAGS			\
> +	TRACE_DEFINE_GFP_FLAGS_GENERAL		\
> +	TRACE_DEFINE_GFP_FLAGS_KASAN		\
> +	TRACE_DEFINE_GFP_FLAGS_LOCKDEP
> +
>  /*

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Linus Torvalds]

On Thu, 16 Jan 2025 at 10:23, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> --- a/include/linux/gfp_types.h
> +++ b/include/linux/gfp_types.h

No - please do these somewhere else.

This header is included by *everything*.

Put it in some tracing header that isn't.

             Linus

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Steven Rostedt]

On Thu, 16 Jan 2025 11:00:20 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 16 Jan 2025 at 10:23, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > --- a/include/linux/gfp_types.h
> > +++ b/include/linux/gfp_types.h  
> 
> No - please do these somewhere else.
> 
> This header is included by *everything*.
> 
> Put it in some tracing header that isn't.
> 

But they are only defines, they are not created until used. Are you worried
that it will slow down the compile?

I wanted this to stay close to where they are crated so they can keep in
sync, and not break when something new is added.

Another way is to have something like:

diff --git a/include/linux/gfp_types.h b/include/linux/gfp_types.h
index 4591a3e1711d..65c61c7b9f22 100644
--- a/include/linux/gfp_types.h
+++ b/include/linux/gfp_types.h
@@ -23,41 +23,66 @@ typedef unsigned int __bitwise gfp_t;
  * include/trace/events/mmflags.h and tools/perf/builtin-kmem.c
  */
 
-enum {
-	___GFP_DMA_BIT,
-	___GFP_HIGHMEM_BIT,
-	___GFP_DMA32_BIT,
-	___GFP_MOVABLE_BIT,
-	___GFP_RECLAIMABLE_BIT,
-	___GFP_HIGH_BIT,
-	___GFP_IO_BIT,
-	___GFP_FS_BIT,
-	___GFP_ZERO_BIT,
-	___GFP_UNUSED_BIT,	/* 0x200u unused */
-	___GFP_DIRECT_RECLAIM_BIT,
-	___GFP_KSWAPD_RECLAIM_BIT,
-	___GFP_WRITE_BIT,
-	___GFP_NOWARN_BIT,
-	___GFP_RETRY_MAYFAIL_BIT,
-	___GFP_NOFAIL_BIT,
-	___GFP_NORETRY_BIT,
-	___GFP_MEMALLOC_BIT,
-	___GFP_COMP_BIT,
-	___GFP_NOMEMALLOC_BIT,
-	___GFP_HARDWALL_BIT,
-	___GFP_THISNODE_BIT,
-	___GFP_ACCOUNT_BIT,
-	___GFP_ZEROTAGS_BIT,
+#define GFP_BITS_GENERAL			\
+	EM(DMA)					\
+	EM(HIGHMEM)				\
+	EM(DMA32)				\
+	EM(MOVABLE)				\
+	EM(RECLAIMABLE)				\
+	EM(HIGH)				\
+	EM(IO)					\
+	EM(FS)					\
+	EM(ZERO)				\
+	EM(UNUSED_BIT)				\
+	EM(DIRECT_RECLAIM)			\
+	EM(KSWAPD_RECLAIM)			\
+	EM(WRITE)				\
+	EM(NOWARN)				\
+	EM(RETRY_MAYFAIL)			\
+	EM(NOFAIL)				\
+	EM(NORETRY)				\
+	EM(MEMALLOC)				\
+	EM(COMP)				\
+	EM(NOMEMALLOC)				\
+	EM(HARDWALL)				\
+	EM(THISNODE)				\
+	EM(ACCOUNT)				\
+	EM(ZEROTAGS)
+
 #ifdef CONFIG_KASAN_HW_TAGS
-	___GFP_SKIP_ZERO_BIT,
-	___GFP_SKIP_KASAN_BIT,
+# define GFP_BITS_KASAN				\
+	EM(SKIP_ZERO)				\
+	EM(SKIP_KASAN)
+#else
+# define GFP_BITS_KASAN
 #endif
+
 #ifdef CONFIG_LOCKDEP
-	___GFP_NOLOCKDEP_BIT,
+# define GFP_BITS_LOCKDEP			\
+	EM(NOLOCKDEP)
+#else
+# define GFP_BITS_LOCKDEP
 #endif
+
 #ifdef CONFIG_SLAB_OBJ_EXT
-	___GFP_NO_OBJ_EXT_BIT,
+# define GFP_BITS_SLAB				\
+	EM(NO_OBJ_EXT)
+#else
+# define GFP_BITS_SLAB
 #endif
+
+# define GFP_BITS				\
+	GFP_BITS_GENERAL			\
+	GFP_BITS_KASAN				\
+	GFP_BITS_LOCKDEP			\
+	GFP_BITS_SLAB
+
+
+#undef EM
+#define EM(a)	___GFP_##a##_BIT,
+
+enum {
+	GFP_BITS
 	___GFP_LAST_BIT
 };
 

Then I could just use the GFP_BITS and redefine the EM() to add the
TRACE_DEFINE_ENUM() around them. Or something similar.

But I didn't think you would go for that.

-- Steve

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Linus Torvalds]

On Thu, 16 Jan 2025 at 11:17, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> But they are only defines, they are not created until used. Are you worried
> that it will slow down the compile?

Yes, and the whole "pointless noise" thing.

I *look* at header files sometimes. I don't want to see pointless
noise that isn't relevant to the context.

This is not relevant to non-tracing users.

                 Linus

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Steven Rostedt]

On Thu, 16 Jan 2025 11:19:20 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 16 Jan 2025 at 11:17, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > But they are only defines, they are not created until used. Are you worried
> > that it will slow down the compile?  
> 
> Yes, and the whole "pointless noise" thing.
> 
> I *look* at header files sometimes. I don't want to see pointless
> noise that isn't relevant to the context.
> 
> This is not relevant to non-tracing users.

Would it at least be OK to add a comment there to update the tracing code
if a new GFP flag is added?

/*
 * If a new GFP flag is added, a new TRACE_DEFINE_ENUM needs to be added
 * in path/to/where/define/is/made.h
 */


?

-- Steve

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Linus Torvalds]

On Thu, 16 Jan 2025 at 11:30, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Would it at least be OK to add a comment there to update the tracing code
> if a new GFP flag is added?

Sure.

             Linus

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Steven Rostedt]

On Thu, 16 Jan 2025 13:23:59 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> --- a/include/trace/events/kmem.h
> +++ b/include/trace/events/kmem.h
> @@ -9,6 +9,8 @@
>  #include <linux/tracepoint.h>
>  #include <trace/events/mmflags.h>
>  
> +TRACE_DEFINE_GFP_FLAGS

I also tested this only with the above define in mmflags.h and just added
it to kmem.h before sending out. It appears there's a bug in the
TRACE_EVENT_ENUM define that doesn't translate the TRACE_SYSTEM properly
and it created the same variable used for both kmem.h and mmflags.h and the
build failed.

So v2 will also include a fix to TRACE_DEFINE_ENUM().

-- Steve


> +
>  TRACE_EVENT(kmem_cache_alloc,
>  
>  	TP_PROTO(unsigned long call_site,
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index bb8a59c6caa2..522bbe3a5fe1 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -15,6 +15,8 @@
>  
>  #define gfpflag_string(flag) {(__force unsigned long)flag, #flag}
>  
> +TRACE_DEFINE_GFP_FLAGS
> +
>  #define __def_gfpflag_names			\
>  	gfpflag_string(GFP_TRANSHUGE),		\
>  	gfpflag_string(GFP_TRANSHUGE_LIGHT),	\
> -- 

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Steven Rostedt]

On Thu, 16 Jan 2025 14:43:41 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> I also tested this only with the above define in mmflags.h and just added
> it to kmem.h before sending out. It appears there's a bug in the
> TRACE_EVENT_ENUM define that doesn't translate the TRACE_SYSTEM properly
> and it created the same variable used for both kmem.h and mmflags.h and the
> build failed.
> 
> So v2 will also include a fix to TRACE_DEFINE_ENUM().

Never mind. It appears that the kmem.h includes mmflags.h :-p

Well, the TRACE_DEFINE_ENUM() still needs to be fixed, but that could wait
for the next merge window.

-- Steve

Re: [PATCH] tracing: gfp: Fix the GFP enum values shown for user space tracing tools

[Steven Rostedt]

On Thu, 16 Jan 2025 11:30:52 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 16 Jan 2025 at 11:30, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Would it at least be OK to add a comment there to update the tracing code
> > if a new GFP flag is added?  
> 
> Sure.

Heh, there's already a comment, and I can make the changes there:


/*
 * In case of changes, please don't forget to update
 * include/trace/events/mmflags.h and tools/perf/builtin-kmem.c
 */

enum {
	___GFP_DMA_BIT,
	___GFP_HIGHMEM_BIT,
[..]

-- Steve