Re: [PATCH v3 3/5] mm: userfaultfd: combine 'mode' and 'wp_copy' arguments

From: Axel Rasmussen <axelrasmussen@google.com>
To: Peter Xu <peterx@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Hugh Dickins <hughd@google.com>, Jan Kara <jack@suse.cz>,
	"Liam R. Howlett" <Liam.Howlett@oracle.com>,
	Matthew Wilcox <willy@infradead.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Mike Rapoport <rppt@kernel.org>,
	Muchun Song <muchun.song@linux.dev>,
	Nadav Amit <namit@vmware.com>, Shuah Khan <shuah@kernel.org>,
	James Houghton <jthoughton@google.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v3 3/5] mm: userfaultfd: combine 'mode' and 'wp_copy' arguments
Date: Tue, 7 Mar 2023 15:27:17 -0800	[thread overview]
Message-ID: <CAJHvVciQWctUoZtrPga-fhgBf2dtc+6ypwE3FYe8ApQWpQyL0Q@mail.gmail.com> (raw)
In-Reply-To: <ZAaMs44nspRQJmrk@x1n>

On Mon, Mar 6, 2023 at 5:00 PM Peter Xu <peterx@redhat.com> 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@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?

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
>