On Mon, Oct 2, 2023 at 12:34 PM Lokesh Gidra lokeshgidra@google.com wrote:
On Mon, Oct 2, 2023 at 6:43 PM David Hildenbrand david@redhat.com wrote:
On 02.10.23 17:55, Lokesh Gidra wrote:
On Mon, Oct 2, 2023 at 4:46 PM Lokesh Gidra lokeshgidra@google.com wrote:
On Mon, Oct 2, 2023 at 4:21 PM Peter Xu peterx@redhat.com wrote:
On Mon, Oct 02, 2023 at 10:00:03AM +0200, David Hildenbrand wrote:
In case we cannot simply remap the page, the fallback sequence (from the cover letter) would be triggered.
- UFFDIO_COPY
- MADV_DONTNEED
So we would just handle the operation internally without a fallback.
Note that I think there will be a slight difference on whole remap atomicity, on what happens if the page is modified after UFFDIO_COPY but before DONTNEED.
UFFDIO_REMAP guarantees full atomicity when moving the page, IOW, threads can be updating the pages when ioctl(UFFDIO_REMAP), data won't get lost during movement, and it will generate a missing event after moved, with latest data showing up on dest.
I'm not sure that means such a fallback is a problem, Suren may know better with the use case.
Although there is no problem in using fallback with our use case but as a user of userfaultfd, I'd suggest leaving it to the developer. Failing with appropriate errno makes more sense. If handled in the kernel, then the user may assume at the end of the operation that the src vma is completely unmapped. And if not correctness issues, it could lead to memory leaks.
I meant that in addition to the possibility of correctness issues due to lack of atomicity, it could also lead to memory leaks, as the user may assume that src vma is empty post-operation. IMHO, it's better to fail with errno so that the user would fix the code with necessary changes (like using DONTFORK, if forking).
Leaving the atomicity discussion out because I think this can just be handled (e.g., the src_vma would always be empty post-operation):
It might not necessarily be a good idea to only expose micro-operations to user space. If the user-space fallback will almost always be "UFFDIO_COPY+MADV_DONTNEED", then clearly the logical operation performed is moving data, ideally with zero-copy.
IMHO, such a fallback will be useful only if it's possible that only some pages in the src vma fail due to this. But even then it would be really useful to have a flag maybe like UFFDIO_REMAP_FALLBACK_COPY to control if the user wants the fallback or not. OTOH, if this is something that can be detected for the entire src vma, then failing with errno is more appropriate.
Given that the patch is already quite complicated, I humbly suggest leaving the fallback for now as a TODO.
Ok, I think it makes sense to implement the strict remap logic but in a way that we can easily add copy fallback if that's needed in the future. So, I'll change UFFDIO_REMAP to UFFDIO_MOVE and will return some unique error, like EBUSY when the page is not PAE. If we need to add a copy fallback in the future, we will add a UFFDIO_MOVE_MODE_ALLOW_COPY flag and will implement the copy mechanism. Does that sound good?
[as said as reply to Peter, one could still have magic flags for users that really want to detect when zero-copy is impossible]
With a logical MOVE API users like compaction [as given in the cover letter], not every such user has to eventually implement fallback paths.
But just my 2 cents, the UFFDIO_REMAP users probably can share what the exact use cases are and if fallbacks are required at all or if no-KSM + DONTFORK just does the trick.
-- Cheers,
David / dhildenb