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.
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.
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()?
