Re: [PATCH RFC] userfaultfd: introduce UFFDIO_COPY_MODE_YOUNG

[Mike Rapoport <rppt@linux.ibm.com>]

On Tue, Jun 14, 2022 at 01:56:56PM -0700, Nadav Amit wrote:
> On Jun 14, 2022, at 1:40 PM, John Hubbard <jhubbard@nvidia.com> wrote:
> 
> > On 6/14/22 11:56, Mike Rapoport wrote:
> >>> But, I cannot take it anymore: the list of arguments for uffd stuff is
> >>> crazy. I would like to collect all the possible arguments that are used for
> >>> uffd operation into some “struct uffd_op”.
> >> Squashing boolean parameters into int flags will also reduce the insane
> >> amount of parameters. No strong feelings though.
> >>  
> > 
> > Just a quick drive-by comment about boolean arguments: they ruin the
> > readability of the call sites. In practice, sometimes a single boolean
> > argument can be OK-ish (still poor to read at the call site, but easier
> > to code initially), but once you get past one boolean argument in the
> > function, readability is hopeless:
> > 
> >    foo(ptr, true, false, a == b);
> > 
> > So if you have a choice, I implore you to prefer flags and/or enums. :)
> 
> Thanks for the feedback - I am aware it is very confusing to have booleans
> and especially multiple ones in a func call.
> 
> Just not sure how it maps to what I proposed. I thought of passing as an
> argument reference (pointer) to something similar to the following struct,
> which I think is very self-descriptive:
> 
> struct uffd_op {
> 	/* various fields */
> 	struct vm_area_struct *dst_vma;
> 	unsigned long len;
> 	atomic_t *mmap_changing;
> 
> 	...
> 	
> 	/* ... and some flags */
> 	int wp: 1;
> 	int zero: 1;
> 	int read_likely: 1;
> 
> 	...
> };
> 
> I think that fits what you were asking for. The only thing I am not sure of,
> is whether to include in uffd_op fields that are internal to mm/userfaultfd
> such as “page” and “newly_allocated”. I guess not.

mfill_atomic_install_pte() is called by shmem_mfill_atomic_pte() so it's
not entirely internal to mm/userfaultfd.c.

Another thing is that with all the parameters packed into a struct, the
call sites could become really hairy, so maybe the best way would be to
pack some of the parameters and leave the others.

But you'll never know until you try :)

-- 
Sincerely yours,
Mike.