On Mon, Mar 06, 2023 at 02:50:22PM -0800, Axel Rasmussen wrote: > Many userfaultfd ioctl functions take both a 'mode' and a 'wp_copy' > argument. In future commits we plan to plumb the flags through to more > places, so we'd be proliferating the very long argument list even > further. > > Let's take the time to simplify the argument list. Combine the two > arguments into one - and generalize, so when we add more flags in the > future, it doesn't imply more function arguments. > > Since the modes (copy, zeropage, continue) are mutually exclusive, store > them as an integer value (0, 1, 2) in the low bits. Place combine-able > flag bits in the high bits. > > This is quite similar to an earlier patch proposed by Nadav Amit > ("userfaultfd: introduce uffd_flags" - for some reason Lore no longer > has a copy of the patch). The main difference is that patch only handled Lore has. :) https://lore.kernel.org/all/20220619233449.181323-2-namit@xxxxxxxxxx And btw sorry to review late. > flags, whereas this patch *also* combines the "mode" argument into the > same type to shorten the argument list. > > Acked-by: James Houghton <jthoughton@xxxxxxxxxx> > Signed-off-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx> Mostly good to me, a few nitpicks below. [...] > +/* A combined operation mode + behavior flags. */ > +typedef unsigned int __bitwise uffd_flags_t; > + > +/* Mutually exclusive modes of operation. */ > +enum mfill_atomic_mode { > + MFILL_ATOMIC_COPY = (__force uffd_flags_t) 0, > + MFILL_ATOMIC_ZEROPAGE = (__force uffd_flags_t) 1, > + MFILL_ATOMIC_CONTINUE = (__force uffd_flags_t) 2, > + NR_MFILL_ATOMIC_MODES, > }; I never used enum like this. I had a feeling that this will enforce setting the enum entries but would the enforce applied to later assignments? I'm not sure. I had a quick test and actually I found sparse already complains about calculating the last enum entry: ---8<--- $ cat a.c typedef unsigned int __attribute__((bitwise)) flags_t; enum { FLAG1 = (__attribute__((force)) flags_t) 0, FLAG_NUM, }; void main(void) { uffd_flags_t flags = FLAG1; } $ sparse a.c a.c:5:5: error: can't increment the last enum member ---8<--- Maybe just use the simple "#define"s? > > +#define MFILL_ATOMIC_MODE_BITS (const_ilog2(NR_MFILL_ATOMIC_MODES - 1) + 1) Here IIUC it should be "const_ilog2(NR_MFILL_ATOMIC_MODES) + 1", but maybe.. we don't bother and define every bit explicitly? > +#define MFILL_ATOMIC_BIT(nr) ((__force uffd_flags_t) BIT(MFILL_ATOMIC_MODE_BITS + (nr))) > +#define MFILL_ATOMIC_MODE_MASK (MFILL_ATOMIC_BIT(0) - 1) > + > +/* Flags controlling behavior. */ > +#define MFILL_ATOMIC_WP MFILL_ATOMIC_BIT(0) [...] > @@ -312,9 +312,9 @@ static __always_inline ssize_t mfill_atomic_hugetlb( > unsigned long dst_start, > unsigned long src_start, > unsigned long len, > - enum mcopy_atomic_mode mode, > - bool wp_copy) > + uffd_flags_t flags) > { > + int mode = flags & MFILL_ATOMIC_MODE_MASK; > struct mm_struct *dst_mm = dst_vma->vm_mm; > int vm_shared = dst_vma->vm_flags & VM_SHARED; > ssize_t err; > @@ -333,7 +333,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb( > * by THP. Since we can not reliably insert a zero page, this > * feature is not supported. > */ > - if (mode == MCOPY_ATOMIC_ZEROPAGE) { > + if (mode == MFILL_ATOMIC_ZEROPAGE) { The mode comes from "& MFILL_ATOMIC_MODE_MASK" but it doesn't quickly tell whether there's a shift for the mask. Would it look better we just have a helper to fetch the mode? The function tells that whatever it returns must be the mode: if (uffd_flags_get_mode(flags) == MFILL_ATOMIC_ZEROPAGE) We also avoid quite a few "mode" variables. All the rest bits will be fine to use "flags & FLAG1" if it's a boolean (so only this "mode" is slightly tricky). What do you think? Thanks, -- Peter Xu