On Mon, Mar 6, 2023 at 5:00 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> 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?
Agreed, if sparse isn't happy with this then using the force macros is
pointless.
The enum is valuable because it lets us get the # of modes; assuming
we agree that's useful below ...
>
> >
> > +#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?
If my reading of const_ilog2's definition is correct, then:
const_ilog2(4) = 2
const_ilog2(3) = 1
const_ilog2(2) = 1
For either 3 or 4 modes, we need 2 bits to represent them (0, 1, 2,
3), i.e. we want MFILL_ATOMIC_MODE_BITS = 2. I think this is correct
as is, because const_ilog2(4 - 1) + 1 = 2, and const_ilog2(3 - 1) + 1
= 2.
In other words, I think const_ilog2 is defined as floor(log2()),
whereas what we want is ceil(log2()).
The benefit of doing this vs. just doing defines with fixed values is,
if we ever added a new mode, we wouldn't have to do bit twiddling and
update the mask, flag bits, etc. - it would happen "automatically". I
prefer it this way, but I agree it is a matter of opinion / taste. :)
If you or others feel strongly this is overcomplicated, I can take the
other approach.
>
> > +#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).
Agreed, this is simpler. I'll make this change.
>
> What do you think?
>
> Thanks,
>
> --
> Peter Xu
>
