On Wed, Jun 15, 2022 at 09:58:27AM -0700, Nadav Amit wrote:
> On Jun 15, 2022, at 8:43 AM, Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> > 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.
>
> I see 3 options:
>
> 1. Pack only fs/mm flags: WP, read-likely, write-likely.
> 2. (1) + as part of the flags internally include Axel’s copy_atomic_mode.
> 3. The uffd_op approach: include all relevant fields.
>
> For the time being I’m going with (1) since I do not have too much time to
> finish all of that and upstream the rest of my work (Broadcom is knocking).
>
> (3) also has the downside of stack-protector that would be added due to
> stack-protector strong, which is not-that-bad, but I hate it.
Any short (but further) explanations?
>
>
> Three more points for consideration in future cleanups:
>
> 1. This __always_inline thingy is crazy IMHO. The size of the compilation
> unit is almost double because of it, and I saw no explanation for its use in
> the commit log (unless I missed it). The overheads in userfaultfd are mostly
> due to memory copying, scheduling, IPIs.
Do you meant __mcopy_atomic()? I agree it's suspicious. That's a huge
function with three callers. Even if to mark it inline I think it's more
proper to mark it at the three callers, or I'd think we can leave that for
the compilers.
>
> 2. I think it makes more sense to strive not to have more than 6 arguments
> for each function (as supported in registers on x86). For that it is possible
> to get rid of dst_mm when it can be retrieved from dst_vma. Anyhow we access
> dst_vma->vm_flags which share a cache-line with dst_vma->vm_mm.
>
> 3. These BUG_ON()s all around are also ... excessive. I guess they were
> introduced before the age in which Linus got angry on each BUG_ON(). Is
> there any good reason not to change them into VM_BUG_ON()?
I'm not sure how that was handled, I'd personally think it's fine to keep
them if they're there for ages. They're gone already if they can be
triggered in any bad ways anyway.. But for sure we'd keep an eye on new
ones when reviewing.
--
Peter Xu
