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

From: Peter Xu <peterx@redhat.com>
To: Axel Rasmussen <axelrasmussen@google.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: Mon, 6 Mar 2023 20:00:35 -0500	[thread overview]
Message-ID: <ZAaMs44nspRQJmrk@x1n> (raw)
In-Reply-To: <20230306225024.264858-4-axelrasmussen@google.com>

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