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
