| Message ID | 20250116132359.1f20cdec@gandalf.local.home (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tracing: gfp: Fix the GFP enum values shown for user space tracing tools | expand |
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 > + > /*
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
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
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
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
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
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), \ > --
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
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
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), \