Messages in this thread Patch in this message | | | Date | Mon, 6 Mar 2023 20:00:35 -0500 | From | Peter Xu <> | Subject | Re: [PATCH v3 3/5] mm: userfaultfd: combine 'mode' and 'wp_copy' arguments |
| |
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@vmware.com
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@google.com> > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
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
| |