Re: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI

[David Hildenbrand <david@redhat.com>]

>>
>> The sub-feature in question (cross-mm) has no solid use cases; at this
>> point I am not even convinced the use case you raised requires
>> *userfaultfd*; for the purpose of moving a whole VMA worth of pages
>> between two processes; I don't see the immediate need to get userfaultfd
>> involved and move individual pages under page lock etc.
> 
> You make a compelling case against cross-mm support.

I tried to :P

I'm happy to hear compelling cases for cross-mm support that we need 
*right now*. And that's what I'm missing so far besides "already 
included in the patches" and "but we would eventually need a separate flag".

As a side note, I already do have another rmap-related feature in the 
works that will require extra-effort to handle this case (short: assign 
each MM a unique ID and use that for accounting purposes when 
(un)mapping pages); I think I figured out how to handle this case here; 
and it's questionable if my work will make it upstream -- to be posted 
as PoC in 2-4 weeks I guess. But it easily shows that there are cases 
where this will require extra work -- without any current benefits due 
to lack of actual users.

> While I can't force Andrea to participate in upstreaming nor do I have
> his background, keeping it simple, as you requested, is doable. That's
> what I plan on doing by splitting the patch and I think we all agreed
> to that. I'll also see if I can easily add a separate patch to test
> cross-mm support.
> I do apologize for the extra effort required from reviewers to cover
> for the gaps in my patches. I'm doing my best to minimize that and I
> really appreciate your time.

It's absolutely not your fault and there is absolutely no need to 
apologize (sorry if I sounded like I would be blaming you in any way). I 
made myself the experience that up-streaming the work of someone else 
can be troublesome, because it's hard to grasp all the details from a 
set of patches. Documentation and comments can't handle all the implicit 
knowledge from the original author.

I likely wouldn't be able to even write that code myself.

For example: why is cross-mm relevant and was included in the original 
patches? Maybe there was a very good reason and it is simply not documented.

> 
>>
>>>
>>> I'll leave that to Suren and Lokesh to decide.  For me the worst case is
>>> one more flag which might be confusing, which is not the end of the world..
>>> Suren, you may need to work more thoroughly to remove cross-mm implications
>>> if so, just like when renaming REMAP to MOVE.
>>
>> I'm asking myself why you are pushing so hard to include complexity
>> "just because we can"; doesn't make any sense to me, honestly.
>>
>> Maybe you have some other real use cases that ultimately require
>> userfaultfd for cross-mm that you cannot share?
>>
>> Will the world end when we have to use a separate flag so we can open
>> this pandora's box when really required?
>>
>>
>> Again, moving anon pages within a process is a known thing; we do that
>> already via mremap; the only difference here really is, that we have to
>> get the rmap right because we don't adjust VMAs. It's a shame we don't
>> try to combine both code paths, maybe it's not easily possible like we
>> did with mprotect vs. uffd-wp.
> 
> That's a good point. With cross-mm support baked in, the overlap was
> not obvious to me. I'll see how much we can reuse from the mremap
> path.

My comment was inspired by Lokesh "While going through mremap's 
move_page_tables code, which is pretty similar to what we do here".

There are some subtle differences (could we even move whole page tables? 
probably not due to holding the mmap locking only in read-mode) and 
special exclusive-only+rmap adjust handling. Further, TLB flushing is 
different (but maybe there are ways to just reuse the batching, did not 
look into the details).

But move_page_tables is clearly single-mm code, and a unification might 
not be that straight forward.

> 
>>
>> Moving anon pages between process is currently only done via COW, where
>> all things (page pinning, memcg, ...) have been figured out and are
>> simply working as expected. Making uffd special by coding-up their own
>> thing does not sound compelling to me.
>>
>>
>> I am clearly against any unwarranted features+complexity. Again, I will
>> stop arguing further, the whole thing of "include it just because we
>> can" to avoid a flag (that we might never even see) doesn't make any
>> sense to me and likely never will.
>>
>> The whole way this feature is getting upstreamed is just messed up IMHO
>> and I the reasoning used in this thread to stick
>> as-close-as-possible to some code person B wrote some years ago (e.g.,
>> naming, sub-features) is far out of my comprehension.
> 
> I don't think staying as-close-as-possible to the original version was
> the way I was driving this so far. At least that was not my conscious

These are rather the vibes I'm getting from Peter. "Why rename it, could 
confuse people because the original patches are old", "Why exclude it if 
it has been included in the original patches". Not the kind of reasoning 
I can relate to when it comes to upstreaming some patches.


> intention. I'm open to further suggestions whenever it makes sense to
> deviate from it.

I'll repeat: any complexity we remove and any code reused in common 
code/moved out of userfaultfd will be a win.

-- 
Cheers,

David / dhildenb