Re: [PATCH] mm: use enum for vm_flags

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Oct 08, 2025 at 01:15:12PM +0000, Alice Ryhl wrote:
> On Wed, Oct 08, 2025 at 12:54:27PM +0000, Jakub Acs wrote:
> > redefine VM_* flag constants with BIT()
> > 
> > Make VM_* flag constant definitions consistent - unify all to use BIT()
> > macro and define them within an enum.
> > 
> > The bindgen tool is better able to handle BIT(_) declarations when used
> > in an enum.
> > 
> > Also add enum definitions for tracepoints.
> > 
> > We have previously changed VM_MERGEABLE in a separate bugfix. This is a
> > follow-up to make all the VM_* flag constant definitions consistent, as
> > suggested by David in [1].
> > 
> > [1]: https://lore.kernel.org/all/85f852f9-8577-4230-adc7-c52e7f479454@redhat.com/
> > 
> > Signed-off-by: Jakub Acs <acsjakub@amazon.de>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Xu Xin <xu.xin16@zte.com.cn>
> > Cc: Chengming Zhou <chengming.zhou@linux.dev>
> > Cc: Peter Xu <peterx@redhat.com>
> > Cc: Axel Rasmussen <axelrasmussen@google.com>
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > 
> > Hi Alice,
> > 
> > thanks for the patch, I squashed it in (should I add your signed-off-by
> > too?) and added the TRACE_DEFINE_ENUM calls pointed out by Derrick.
> 
> You could add this if you go with the enum approach:
> 
> Co-Developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> 
> > I have the following points to still address, though: 
> > 
> > - can the fact that we're not controlling the type of the values if
> >   using enum be a problem? (likely the indirect control we have through
> >   the highest value is good enough, but I'm not sure)
> 
> The compiler should pick the right integer type in this case.
> 
> > - where do TRACE_DEFINE_ENUM calls belong?
> >   I see them placed e.g. in include/trace/misc/nfs.h for nfs or
> >   arch/x86/kvm/mmu/mmutrace.h, but I don't see a corresponding file for
> >   mm.h - does this warrant creating a separate file for these
> >   definitions?
> > 
> > - with the need for TRACE_DEFINE_ENUM calls, do we still deem this
> >   to be a good trade-off? - isn't fixing all of these in
> >   rust/bindings/bindings_helper.h better?
> > 
> > @Derrick, can you point me to how to test for the issue you pointed out?
> 
> I'm not familiar with the TRACE_DEFINE_ENUM unfortunately.

rostedt already filled in the technical details, so I can supply an
example from code in XFS:

$ git grep -E '(XFS_REFC_DOMAIN_COW|XFS_REFC_DOMAIN_STRINGS)' fs/xfs/
fs/xfs/libxfs/xfs_refcount.c:118:               irec->rc_domain = XFS_REFC_DOMAIN_COW;
<snip>
fs/xfs/libxfs/xfs_types.h:164:  XFS_REFC_DOMAIN_COW,
fs/xfs/libxfs/xfs_types.h:167:#define XFS_REFC_DOMAIN_STRINGS \
fs/xfs/libxfs/xfs_types.h:169:  { XFS_REFC_DOMAIN_COW,          "cow" }
<snip>
fs/xfs/xfs_trace.h:1117:TRACE_DEFINE_ENUM(XFS_REFC_DOMAIN_COW);
fs/xfs/xfs_trace.h:3635:                  __print_symbolic(__entry->domain, XFS_REFC_DOMAIN_STRINGS),

XFS_REFC_DOMAIN_COW is part of an enumeration:

enum xfs_refc_domain {
	XFS_REFC_DOMAIN_SHARED = 0,
	XFS_REFC_DOMAIN_COW,
};

Which then has a string decoder macro defined for use in
__print_symbolic:

#define XFS_REFC_DOMAIN_STRINGS \
	{ XFS_REFC_DOMAIN_SHARED,	"shared" }, \
	{ XFS_REFC_DOMAIN_COW,		"cow" }

Note the TRACE_DEFINE_ENUM usage in the grep output.

Let's look at one of the tracepoints that uses XFS_REFC_DOMAIN_STRINGS.
The class is xfs_refcount_extent_class, so a relevant tracepoint is
xfs_refcount_get:

# cat /sys/kernel/tracing/events/xfs/xfs_refcount_get/format
name: xfs_refcount_get
ID: 1839
format:
        field:unsigned short common_type;       offset:0;       size:2; signed:0;
        field:unsigned char common_flags;       offset:2;       size:1; signed:0;
        field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
        field:int common_pid;   offset:4;       size:4; signed:1;

        field:dev_t dev;        offset:8;       size:4; signed:0;
        field:enum xfs_group_type type; offset:12;      size:1; signed:0;
        field:xfs_agnumber_t agno;      offset:16;      size:4; signed:0;
        field:enum xfs_refc_domain domain;      offset:20;      size:4; signed:0;
        field:xfs_agblock_t startblock; offset:24;      size:4; signed:0;
        field:xfs_extlen_t blockcount;  offset:28;      size:4; signed:0;
        field:xfs_nlink_t refcount;     offset:32;      size:4; signed:0;

print fmt: "dev %d:%d %sno 0x%x dom %s gbno 0x%x fsbcount 0x%x refcount %u", ((unsigned int) ((REC->dev) >> 20)), ((unsigned int) ((REC->dev) & ((1U << 20) - 1))), __print_symbolic(REC->type, { 0, "ag" }, { 1, "rtg" }), REC->agno, __print_symbolic(REC->domain, { 0, "shared" }, { 1, "cow" }), REC->startblock, REC->blockcount, REC->refcount

Notice that the XFS_REFC_DOMAIN_* enumeration values have been
converted into their raw numeric form inside the __print_symbolic
construction so that they're ready for trace-cmd-report.

It's really helpful to have ftrace render bitfield and enumeration
"integers" into something human-readable, especially in filesystems
where there are a lot of those.

--D

> > +#ifndef CONFIG_MMU
> > +TRACE_DEFINE_ENUM(VM_MAYOVERLAY);
> > +#endif /* CONFIG_MMU */
> 
> Here I think you want:
> 
> #ifdef CONFIG_MMU
> TRACE_DEFINE_ENUM(VM_UFFD_MISSING);
> #else
> TRACE_DEFINE_ENUM(VM_MAYOVERLAY);
> #endif /* CONFIG_MMU */
> 
> > +TRACE_DEFINE_ENUM(VM_SOFTDIRTY);
> 
> Here I think you want:
> 
> #ifdef CONFIG_MEM_SOFT_DIRTY
> TRACE_DEFINE_ENUM(VM_SOFTDIRTY);
> #endif
> 
> Alice
>