Re: [PATCH RFC] userfaultfd: introduce UFFDIO_COPY_MODE_YOUNG

[Peter Xu <peterx@redhat.com>]

On Wed, Jun 15, 2022 at 10:26:21AM +0300, Mike Rapoport wrote:
> 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 :)

Yeh.  Axel packed some booleans in f619147104c8e into mcopy_atomic_mode.
The other option (besides uffd_ops) could be making mcopy_atomic_mode a
bitmask and keep the rest, the mode itself only took 2 bits.

uffd_ops sounds good too if the final outcome looks clean, since we do pass
quite a few things over and over deep into the stack.

Thanks,

-- 
Peter Xu