| Message ID | 20210408234327.624367-1-axelrasmussen@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | userfaultfd: add minor fault handling for shmem | expand |
On Thu, 8 Apr 2021 16:43:18 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote: > The idea is that it will apply cleanly to akpm's tree, *replacing* the following > patches (i.e., drop these first, and then apply this series): > > userfaultfd-support-minor-fault-handling-for-shmem.patch > userfaultfd-support-minor-fault-handling-for-shmem-fix.patch > userfaultfd-support-minor-fault-handling-for-shmem-fix-2.patch > userfaultfd-support-minor-fault-handling-for-shmem-fix-3.patch > userfaultfd-support-minor-fault-handling-for-shmem-fix-4.patch > userfaultfd-selftests-use-memfd_create-for-shmem-test-type.patch > userfaultfd-selftests-create-alias-mappings-in-the-shmem-test.patch > userfaultfd-selftests-reinitialize-test-context-in-each-test.patch > userfaultfd-selftests-exercise-minor-fault-handling-shmem-support.patch Well. the problem is, > + if (area_alias == MAP_FAILED) > + err("mmap of memfd alias failed"); `err' doesn't exist until eleventy patches later, in Peter's "userfaultfd/selftests: unify error handling". I got tired of (and lost confidence in) replacing "err(...)" with "fprintf(stderr, ...); exit(1)" everywhere then fixing up the fallout when Peter's patch came along. Shudder. Sorry, all this material pretty clearly isn't going to make 5.12 (potentially nine days hence), so I shall drop all the userfaultfd patches. Let's take a fresh run at all of this after -rc1. I have tentatively retained the first series: userfaultfd-add-minor-fault-registration-mode.patch userfaultfd-add-minor-fault-registration-mode-fix.patch userfaultfd-disable-huge-pmd-sharing-for-minor-registered-vmas.patch userfaultfd-hugetlbfs-only-compile-uffd-helpers-if-config-enabled.patch userfaultfd-add-uffdio_continue-ioctl.patch userfaultfd-update-documentation-to-describe-minor-fault-handling.patch userfaultfd-selftests-add-test-exercising-minor-fault-handling.patch but I don't believe they have had much testing standalone, without the other userfaultfd patches present. So I don't think it's smart to upstream these in this cycle. Or I could drop them so you and Peter can have a clean shot at redoing the whole thing. Please let me know.
On Thu, Apr 8, 2021 at 10:04 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 8 Apr 2021 16:43:18 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote: > > > The idea is that it will apply cleanly to akpm's tree, *replacing* the following > > patches (i.e., drop these first, and then apply this series): > > > > userfaultfd-support-minor-fault-handling-for-shmem.patch > > userfaultfd-support-minor-fault-handling-for-shmem-fix.patch > > userfaultfd-support-minor-fault-handling-for-shmem-fix-2.patch > > userfaultfd-support-minor-fault-handling-for-shmem-fix-3.patch > > userfaultfd-support-minor-fault-handling-for-shmem-fix-4.patch > > userfaultfd-selftests-use-memfd_create-for-shmem-test-type.patch > > userfaultfd-selftests-create-alias-mappings-in-the-shmem-test.patch > > userfaultfd-selftests-reinitialize-test-context-in-each-test.patch > > userfaultfd-selftests-exercise-minor-fault-handling-shmem-support.patch > > Well. the problem is, > > > + if (area_alias == MAP_FAILED) > > + err("mmap of memfd alias failed"); > > `err' doesn't exist until eleventy patches later, in Peter's > "userfaultfd/selftests: unify error handling". I got tired of (and > lost confidence in) replacing "err(...)" with "fprintf(stderr, ...); > exit(1)" everywhere then fixing up the fallout when Peter's patch came > along. Shudder. Oof - sorry about that! > > Sorry, all this material pretty clearly isn't going to make 5.12 > (potentially nine days hence), so I shall drop all the userfaultfd > patches. Let's take a fresh run at all of this after -rc1. That's okay, my understanding was already that it certainly wouldn't be in the 5.12 release, but that we might be ready in time for 5.13. > > > I have tentatively retained the first series: > > userfaultfd-add-minor-fault-registration-mode.patch > userfaultfd-add-minor-fault-registration-mode-fix.patch > userfaultfd-disable-huge-pmd-sharing-for-minor-registered-vmas.patch > userfaultfd-hugetlbfs-only-compile-uffd-helpers-if-config-enabled.patch > userfaultfd-add-uffdio_continue-ioctl.patch > userfaultfd-update-documentation-to-describe-minor-fault-handling.patch > userfaultfd-selftests-add-test-exercising-minor-fault-handling.patch > > but I don't believe they have had much testing standalone, without the > other userfaultfd patches present. So I don't think it's smart to > upstream these in this cycle. Or I could drop them so you and Peter > can have a clean shot at redoing the whole thing. Please let me know. From my perspective, both Peter's error handling and the hugetlbfs minor faulting patches are ready to go. (Peter's most importantly; we should establish that as a base, and put all the burden on resolving conflicts with it on us instead of you :).) My memory was that Peter's patch was applied before my shmem series, but it seems I was mistaken. So, maybe the best thing to do is to have Peter send a version of it based on your tree, without the shmem series? And then I'll resolve any conflicts in my tree? It's true that we haven't tested the hugetlbfs minor faults patch extensively *with the shmem one also applied*, but it has had more thorough review than the shmem one at this point (e.g. by Mike Kravetz), and they're rather separate code paths (I'd be surprised if one breaks the other). >
On Fri, Apr 09, 2021 at 10:03:53AM -0700, Axel Rasmussen wrote: > On Thu, Apr 8, 2021 at 10:04 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Thu, 8 Apr 2021 16:43:18 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote: > > > > > The idea is that it will apply cleanly to akpm's tree, *replacing* the following > > > patches (i.e., drop these first, and then apply this series): > > > > > > userfaultfd-support-minor-fault-handling-for-shmem.patch > > > userfaultfd-support-minor-fault-handling-for-shmem-fix.patch > > > userfaultfd-support-minor-fault-handling-for-shmem-fix-2.patch > > > userfaultfd-support-minor-fault-handling-for-shmem-fix-3.patch > > > userfaultfd-support-minor-fault-handling-for-shmem-fix-4.patch > > > userfaultfd-selftests-use-memfd_create-for-shmem-test-type.patch > > > userfaultfd-selftests-create-alias-mappings-in-the-shmem-test.patch > > > userfaultfd-selftests-reinitialize-test-context-in-each-test.patch > > > userfaultfd-selftests-exercise-minor-fault-handling-shmem-support.patch > > > > Well. the problem is, > > > > > + if (area_alias == MAP_FAILED) > > > + err("mmap of memfd alias failed"); > > > > `err' doesn't exist until eleventy patches later, in Peter's > > "userfaultfd/selftests: unify error handling". I got tired of (and > > lost confidence in) replacing "err(...)" with "fprintf(stderr, ...); > > exit(1)" everywhere then fixing up the fallout when Peter's patch came > > along. Shudder. > > Oof - sorry about that! > > > > > Sorry, all this material pretty clearly isn't going to make 5.12 > > (potentially nine days hence), so I shall drop all the userfaultfd > > patches. Let's take a fresh run at all of this after -rc1. > > That's okay, my understanding was already that it certainly wouldn't > be in the 5.12 release, but that we might be ready in time for 5.13. > > > > > > > I have tentatively retained the first series: > > > > userfaultfd-add-minor-fault-registration-mode.patch > > userfaultfd-add-minor-fault-registration-mode-fix.patch > > userfaultfd-disable-huge-pmd-sharing-for-minor-registered-vmas.patch > > userfaultfd-hugetlbfs-only-compile-uffd-helpers-if-config-enabled.patch > > userfaultfd-add-uffdio_continue-ioctl.patch > > userfaultfd-update-documentation-to-describe-minor-fault-handling.patch > > userfaultfd-selftests-add-test-exercising-minor-fault-handling.patch > > > > but I don't believe they have had much testing standalone, without the > > other userfaultfd patches present. So I don't think it's smart to > > upstream these in this cycle. Or I could drop them so you and Peter > > can have a clean shot at redoing the whole thing. Please let me know. > > From my perspective, both Peter's error handling and the hugetlbfs > minor faulting patches are ready to go. (Peter's most importantly; we > should establish that as a base, and put all the burden on resolving > conflicts with it on us instead of you :).) > > My memory was that Peter's patch was applied before my shmem series, > but it seems I was mistaken. So, maybe the best thing to do is to have > Peter send a version of it based on your tree, without the shmem > series? And then I'll resolve any conflicts in my tree? > > It's true that we haven't tested the hugetlbfs minor faults patch > extensively *with the shmem one also applied*, but it has had more > thorough review than the shmem one at this point (e.g. by Mike > Kravetz), and they're rather separate code paths (I'd be surprised if > one breaks the other). Yes I think the hugetlb part should have got more review done. IMHO it's a matter of whether Mike would still like to do a more thorough review, or seems okay to keep them. I can repost the selftest series later if needed, as long as I figured which is the suitable base commit. Those selftest patches are definitely not urgent for this release, so we can wait for the next release. Thanks,
On 4/9/21 2:18 PM, Peter Xu wrote: > On Fri, Apr 09, 2021 at 10:03:53AM -0700, Axel Rasmussen wrote: >> On Thu, Apr 8, 2021 at 10:04 PM Andrew Morton <akpm@linux-foundation.org> wrote: >>> >>> On Thu, 8 Apr 2021 16:43:18 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote: >>> >>>> The idea is that it will apply cleanly to akpm's tree, *replacing* the following >>>> patches (i.e., drop these first, and then apply this series): >>>> >>>> userfaultfd-support-minor-fault-handling-for-shmem.patch >>>> userfaultfd-support-minor-fault-handling-for-shmem-fix.patch >>>> userfaultfd-support-minor-fault-handling-for-shmem-fix-2.patch >>>> userfaultfd-support-minor-fault-handling-for-shmem-fix-3.patch >>>> userfaultfd-support-minor-fault-handling-for-shmem-fix-4.patch >>>> userfaultfd-selftests-use-memfd_create-for-shmem-test-type.patch >>>> userfaultfd-selftests-create-alias-mappings-in-the-shmem-test.patch >>>> userfaultfd-selftests-reinitialize-test-context-in-each-test.patch >>>> userfaultfd-selftests-exercise-minor-fault-handling-shmem-support.patch >>> >>> Well. the problem is, >>> >>>> + if (area_alias == MAP_FAILED) >>>> + err("mmap of memfd alias failed"); >>> >>> `err' doesn't exist until eleventy patches later, in Peter's >>> "userfaultfd/selftests: unify error handling". I got tired of (and >>> lost confidence in) replacing "err(...)" with "fprintf(stderr, ...); >>> exit(1)" everywhere then fixing up the fallout when Peter's patch came >>> along. Shudder. >> >> Oof - sorry about that! >> >>> >>> Sorry, all this material pretty clearly isn't going to make 5.12 >>> (potentially nine days hence), so I shall drop all the userfaultfd >>> patches. Let's take a fresh run at all of this after -rc1. >> >> That's okay, my understanding was already that it certainly wouldn't >> be in the 5.12 release, but that we might be ready in time for 5.13. >> >>> >>> >>> I have tentatively retained the first series: >>> >>> userfaultfd-add-minor-fault-registration-mode.patch >>> userfaultfd-add-minor-fault-registration-mode-fix.patch >>> userfaultfd-disable-huge-pmd-sharing-for-minor-registered-vmas.patch >>> userfaultfd-hugetlbfs-only-compile-uffd-helpers-if-config-enabled.patch >>> userfaultfd-add-uffdio_continue-ioctl.patch >>> userfaultfd-update-documentation-to-describe-minor-fault-handling.patch >>> userfaultfd-selftests-add-test-exercising-minor-fault-handling.patch >>> >>> but I don't believe they have had much testing standalone, without the >>> other userfaultfd patches present. So I don't think it's smart to >>> upstream these in this cycle. Or I could drop them so you and Peter >>> can have a clean shot at redoing the whole thing. Please let me know. >> >> From my perspective, both Peter's error handling and the hugetlbfs >> minor faulting patches are ready to go. (Peter's most importantly; we >> should establish that as a base, and put all the burden on resolving >> conflicts with it on us instead of you :).) >> >> My memory was that Peter's patch was applied before my shmem series, >> but it seems I was mistaken. So, maybe the best thing to do is to have >> Peter send a version of it based on your tree, without the shmem >> series? And then I'll resolve any conflicts in my tree? >> >> It's true that we haven't tested the hugetlbfs minor faults patch >> extensively *with the shmem one also applied*, but it has had more >> thorough review than the shmem one at this point (e.g. by Mike >> Kravetz), and they're rather separate code paths (I'd be surprised if >> one breaks the other). > > Yes I think the hugetlb part should have got more review done. IMHO it's a > matter of whether Mike would still like to do a more thorough review, or seems > okay to keep them. I looked pretty closely at the hugetlb specific parts of the minor fault handling series. I only took a high level look at the code modifying and dealing with the userfaultfd API. The hugetlb specific parts looked fine to me. I can take a closer look at the userfaultfd API modifications, but it would take more time for me to come up to speed on the APIs.
Base ==== Since the original series [1] was merged into Andrew's tree, some issues were noticed. Up to this point, we had been working on fixing what's in Andrew's tree [2], but at this point we've changed direction enough that a lot of the fix's delta is undoing what was done in the original series, thereby making it hard to review. As suggested by Hugh Dickins and Peter Xu, this series takes a step back. It can be considered a v3 of the original series [1] - it combines those patches with the fixes, reordered / broken up to allow for easier review. The idea is that it will apply cleanly to akpm's tree, *replacing* the following patches (i.e., drop these first, and then apply this series): userfaultfd-support-minor-fault-handling-for-shmem.patch userfaultfd-support-minor-fault-handling-for-shmem-fix.patch userfaultfd-support-minor-fault-handling-for-shmem-fix-2.patch userfaultfd-support-minor-fault-handling-for-shmem-fix-3.patch userfaultfd-support-minor-fault-handling-for-shmem-fix-4.patch userfaultfd-selftests-use-memfd_create-for-shmem-test-type.patch userfaultfd-selftests-create-alias-mappings-in-the-shmem-test.patch userfaultfd-selftests-reinitialize-test-context-in-each-test.patch userfaultfd-selftests-exercise-minor-fault-handling-shmem-support.patch Changelog ========= Changes since the most recent fixup patch [2]: - Squash the fixes ([2]) in with the original series ([1]). This makes reviewing easier, as we no longer have to sift through deltas undoing what we had done before. [Hugh, Peter] - Modify shmem_mcopy_atomic_pte() to use the new mcopy_atomic_install_ptes() helper, reducing code duplication. [Hugh] - Properly trigger handle_userfault() in the shmem_swapin_page() case. [Hugh] - Use shmem_getpage() instead of find_lock_page() to lookup the existing page in for continue. This properly deals with swapped-out pages. [Hugh] - Unconditionally pte_mkdirty() for anon memory (as before). [Peter] - Don't include userfaultfd_k.h in either hugetlb.h or shmem_fs.h. [Hugh] - Add comment for UFFD_FEATURE_MINOR_SHMEM (to match _HUGETLBFS). [Hugh] - Fix some small cleanup issues (parens, reworded conditionals, reduced plumbing of some parameters, simplify labels/gotos, ...). [Hugh, Peter] Overview ======== See the series which added minor faults for hugetlbfs [3] for a detailed overview of minor fault handling in general. This series adds the same support for shmem-backed areas. This series is structured as follows: - Commits 1 and 2 are cleanups. - Commits 3 and 4 implement the new feature (minor fault handling for shmem). - Commits 5, 6, 7, 8 update the userfaultfd selftest to exercise the feature. - Commit 9 is one final cleanup, modifying an existing code path to re-use a new helper we've introduced. We rely on the selftest to show that this change doesn't break anything. Use Case ======== In some cases it is useful to have VM memory backed by tmpfs instead of hugetlbfs. So, this feature will be used to support the same VM live migration use case described in my original series. Additionally, Android folks (Lokesh Gidra <lokeshgidra@google.com>) hope to optimize the Android Runtime garbage collector using this feature: "The plan is to use userfaultfd for concurrently compacting the heap. With this feature, the heap can be shared-mapped at another location where the GC-thread(s) could continue the compaction operation without the need to invoke userfault ioctl(UFFDIO_COPY) each time. OTOH, if and when Java threads get faults on the heap, UFFDIO_CONTINUE can be used to resume execution. Furthermore, this feature enables updating references in the 'non-moving' portion of the heap efficiently. Without this feature, uneccessary page copying (ioctl(UFFDIO_COPY)) would be required." [1] https://lore.kernel.org/patchwork/cover/1388144/ [2] https://lore.kernel.org/patchwork/patch/1408161/ [3] https://lore.kernel.org/linux-fsdevel/20210301222728.176417-1-axelrasmussen@google.com/T/#t Axel Rasmussen (9): userfaultfd/hugetlbfs: avoid including userfaultfd_k.h in hugetlb.h userfaultfd/shmem: combine shmem_{mcopy_atomic,mfill_zeropage}_pte userfaultfd/shmem: support minor fault registration for shmem userfaultfd/shmem: support UFFDIO_CONTINUE for shmem userfaultfd/selftests: use memfd_create for shmem test type userfaultfd/selftests: create alias mappings in the shmem test userfaultfd/selftests: reinitialize test context in each test userfaultfd/selftests: exercise minor fault handling shmem support userfaultfd/shmem: modify shmem_mcopy_atomic_pte to use install_ptes fs/userfaultfd.c | 6 +- include/linux/hugetlb.h | 5 +- include/linux/shmem_fs.h | 15 +- include/linux/userfaultfd_k.h | 5 + include/uapi/linux/userfaultfd.h | 7 +- mm/hugetlb.c | 1 + mm/memory.c | 8 +- mm/shmem.c | 122 ++++------ mm/userfaultfd.c | 183 ++++++++++----- tools/testing/selftests/vm/userfaultfd.c | 280 +++++++++++++++-------- 10 files changed, 387 insertions(+), 245 deletions(-) -- 2.31.1.295.g9ea45b61b8-goog