Re: [PATCH RFC] userfaultfd: introduce UFFDIO_COPY_MODE_YOUNG

[Nadav Amit <nadav.amit@gmail.com>]

On Jun 14, 2022, at 2:40 PM, John Hubbard <jhubbard@nvidia.com> wrote:

> On 6/14/22 13:56, Nadav Amit wrote:
> ...
>>> 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 am more accustomed to seeing:
> 	unsigned int flags;
> 
> ...and then some #defines or enums nearby that are used for .flags.
> The bitfields are not used as much, Linus wrote some words about why,
> (which I'm not hopeful I can find). Basically they are not a very
> robust C feature, and the kernel has good support for dealing with
> flags within a word.
> 

Yes, Linus wrote some harsh words about it to me specifically. But my
understanding was the he opposes to the use of bitfields if they are only
used for flags. In this case it is not only flags, so there are not
problems, for instance in initialization of parameter. There are many
instances for such use (e.g., tcp_options_received)..

>> ...
>> };
>> 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.
> 
> Actually, I think passing around a struct might be overkill, when you can
> simply collapse the various boolean args into a single flags arg. It looked
> like a lot of the new args were bools...

Ok. Whatever you prefer. I thought that having something similar to “struct
vm_fault” makes sense, especially since it would allow to avoid propagating
ugly arguments like mmap_changing. But I do not care enough to argue.