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@xxxxxxxxxx> 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
