Re: [PATCH v2 09/13] mm: update all remaining mmap_prepare users to use vma_flags_t
Lorenzo Stoakes <[email protected]>
| Newsgroups | org.kernel.vger.linux-sgx,dev.linux.lists.ntfs3,dev.linux.lists.nvdimm,org.freedesktop.lists.dri-devel,org.freedesktop.lists.intel-gfx,org.kernel.vger.keyrings,org.kernel.vger.linux-cxl,org.kernel.vger.linux-ext4,org.kernel.vger.linux-fsdevel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-security-module,org.kernel.vger.linux-xfs,org.kvack.linux-mm,org.ozlabs.lists.linux-erofs |
|---|---|
| Message-ID | <[email protected]> |
On Fri, Feb 06, 2026 at 11:31:53AM -0800, Andrew Morton wrote: > On Fri, 6 Feb 2026 17:46:36 +0000 Pedro Falcato <[email protected]> wrote: > > > > -#define VM_REMAP_FLAGS (VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP) > > > +#define VMA_REMAP_FLAGS mk_vma_flags(VMA_IO_BIT, VMA_PFNMAP_BIT, \ > > > + VMA_DONTEXPAND_BIT, VMA_DONTDUMP_BIT) > > > > as a sidenote, these flags are no longer constant expressions and thus > > > > static vma_flags_t flags = VMA_REMAP_FLAGS; I mean this would be a code smell anyway :) but point taken. > > > > can't compile. > > Yup, that isn't nice. An all-caps thing with no () is a compile-time > constant. There is precedence for this, e.g. TASK_SIZE_MAX and other arch defines like that: error: initializer element is not a compile-time constant 3309 | static unsigned long task_max = TASK_SIZE_MAX; | ^~~~~~~~~~~~~ And this will almost certainly (and certainly in everything I tested) become a compile-time constant via the optimiser so to all intents and purposes it _is_ essentially compile-time. But the point of doing it this way is to maintain, as much as possible, one-to-one translation between the previous approach and the new with as little noise/friction as possible. Making this a function makes things really horrible honestly. Because vma_remap_flags() suddenly because a vague thing - I'd assume this was a function doing something. So now do we call it get_vma_remap_flags()? Suddenly something nice-ish like: if (vma_flags_test(flags, VMA_REMAP_FLAGS)) { ... } Become: if (vma_flags_test(flags, get_vma_remap_flags())) { ... } And now it's SUPER ambiguous as to what you're doing there. I'd assume right away that get_vma_remap_flags() was going off and doing something or referencing a static variable or something. Given the compile will treat the former _exactly_ as if it were a compile-time constant it's just adding unnecessary ambiguity. So is it something we can live with? If it looks like a duck, walks like a duck and quacks like a duck, but isn't there when the pond is first dug out, can we still call it a duck? ;) > > It looks like we can make this a nice inlined (commented!) lower-cased > C function as a little low-priority cleanup. > > > Rest LGTM though. > > > > Acked-by: Pedro Falcato <[email protected]> > > Great, thanks. Cheers, Lorenzo