| Message ID | 20231121171643.3719880-6-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | userfaultfd move option | expand |
On 21/11/2023 17:16, Suren Baghdasaryan wrote: > Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > into destination buffer while checking the contents of both after > the move. After the operation the content of the destination buffer > should match the original source buffer's content while the source > buffer should be zeroed. Separate tests are designed for PMD aligned and > unaligned cases because they utilize different code paths in the kernel. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > tools/testing/selftests/mm/uffd-common.c | 24 +++ > tools/testing/selftests/mm/uffd-common.h | 1 + > tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > 3 files changed, 214 insertions(+) > > diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c > index fb3bbc77fd00..b0ac0ec2356d 100644 > --- a/tools/testing/selftests/mm/uffd-common.c > +++ b/tools/testing/selftests/mm/uffd-common.c > @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > return __copy_page(ufd, offset, false, wp); > } > > +int move_page(int ufd, unsigned long offset, unsigned long len) > +{ > + struct uffdio_move uffdio_move; > + > + if (offset + len > nr_pages * page_size) > + err("unexpected offset %lu and length %lu\n", offset, len); > + uffdio_move.dst = (unsigned long) area_dst + offset; > + uffdio_move.src = (unsigned long) area_src + offset; > + uffdio_move.len = len; > + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > + uffdio_move.move = 0; > + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > + /* real retval in uffdio_move.move */ > + if (uffdio_move.move != -EEXIST) > + err("UFFDIO_MOVE error: %"PRId64, > + (int64_t)uffdio_move.move); Hi Suren, FYI this error is triggering in mm-unstable (715b67adf4c8): Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648) I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct. Thanks, Ryan > + wake_range(ufd, uffdio_move.dst, len); > + } else if (uffdio_move.move != len) { > + err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move); > + } else > + return 1; > + return 0; > +}
On Fri, Dec 1, 2023 at 1:29 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > > On 21/11/2023 17:16, Suren Baghdasaryan wrote: > > Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > > into destination buffer while checking the contents of both after > > the move. After the operation the content of the destination buffer > > should match the original source buffer's content while the source > > buffer should be zeroed. Separate tests are designed for PMD aligned and > > unaligned cases because they utilize different code paths in the kernel. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > tools/testing/selftests/mm/uffd-common.c | 24 +++ > > tools/testing/selftests/mm/uffd-common.h | 1 + > > tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > > 3 files changed, 214 insertions(+) > > > > diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c > > index fb3bbc77fd00..b0ac0ec2356d 100644 > > --- a/tools/testing/selftests/mm/uffd-common.c > > +++ b/tools/testing/selftests/mm/uffd-common.c > > @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > > return __copy_page(ufd, offset, false, wp); > > } > > > > +int move_page(int ufd, unsigned long offset, unsigned long len) > > +{ > > + struct uffdio_move uffdio_move; > > + > > + if (offset + len > nr_pages * page_size) > > + err("unexpected offset %lu and length %lu\n", offset, len); > > + uffdio_move.dst = (unsigned long) area_dst + offset; > > + uffdio_move.src = (unsigned long) area_src + offset; > > + uffdio_move.len = len; > > + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > > + uffdio_move.move = 0; > > + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > > + /* real retval in uffdio_move.move */ > > + if (uffdio_move.move != -EEXIST) > > + err("UFFDIO_MOVE error: %"PRId64, > > + (int64_t)uffdio_move.move); > > Hi Suren, > > FYI this error is triggering in mm-unstable (715b67adf4c8): > > Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > @uffd-common.c:648) > > I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > happy to go deeper if you can direct. Hi Ryan, Thanks for reporting! Could you please share your kernel config file? There are several places UFFDIO_MOVE returns EBUSY: 4 places in move_pages_huge_pmd(), 2 places in move_present_pte(), 2 places in move_pages_pte() and once in move_swap_pte(). While I'm trying to reproduce, it would be useful if you could check which place is triggering the error. Thanks, Suren. > > Thanks, > Ryan > > > > + wake_range(ufd, uffdio_move.dst, len); > > + } else if (uffdio_move.move != len) { > > + err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move); > > + } else > > + return 1; > > + return 0; > > +} > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On 01.12.23 10:29, Ryan Roberts wrote: > On 21/11/2023 17:16, Suren Baghdasaryan wrote: >> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >> into destination buffer while checking the contents of both after >> the move. After the operation the content of the destination buffer >> should match the original source buffer's content while the source >> buffer should be zeroed. Separate tests are designed for PMD aligned and >> unaligned cases because they utilize different code paths in the kernel. >> >> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >> --- >> tools/testing/selftests/mm/uffd-common.c | 24 +++ >> tools/testing/selftests/mm/uffd-common.h | 1 + >> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >> 3 files changed, 214 insertions(+) >> >> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c >> index fb3bbc77fd00..b0ac0ec2356d 100644 >> --- a/tools/testing/selftests/mm/uffd-common.c >> +++ b/tools/testing/selftests/mm/uffd-common.c >> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >> return __copy_page(ufd, offset, false, wp); >> } >> >> +int move_page(int ufd, unsigned long offset, unsigned long len) >> +{ >> + struct uffdio_move uffdio_move; >> + >> + if (offset + len > nr_pages * page_size) >> + err("unexpected offset %lu and length %lu\n", offset, len); >> + uffdio_move.dst = (unsigned long) area_dst + offset; >> + uffdio_move.src = (unsigned long) area_src + offset; >> + uffdio_move.len = len; >> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >> + uffdio_move.move = 0; >> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >> + /* real retval in uffdio_move.move */ >> + if (uffdio_move.move != -EEXIST) >> + err("UFFDIO_MOVE error: %"PRId64, >> + (int64_t)uffdio_move.move); > > Hi Suren, > > FYI this error is triggering in mm-unstable (715b67adf4c8): > > Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > @uffd-common.c:648) > > I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > happy to go deeper if you can direct. Does it trigger reliably? Which pagesize is that kernel using? I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size. That, however, does not necessarily correspond to the THP size. That one can be obtained using read_pmd_pagesize() in vm_util.c I quickly scanned the code (still want to take a deeper look), but all PAE checks looked sane to me. I think the issue is folio split handling. I replied to the patch.
On Fri, Dec 1, 2023 at 12:47 PM David Hildenbrand <david@redhat.com> wrote: > > On 01.12.23 10:29, Ryan Roberts wrote: > > On 21/11/2023 17:16, Suren Baghdasaryan wrote: > >> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > >> into destination buffer while checking the contents of both after > >> the move. After the operation the content of the destination buffer > >> should match the original source buffer's content while the source > >> buffer should be zeroed. Separate tests are designed for PMD aligned and > >> unaligned cases because they utilize different code paths in the kernel. > >> > >> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >> --- > >> tools/testing/selftests/mm/uffd-common.c | 24 +++ > >> tools/testing/selftests/mm/uffd-common.h | 1 + > >> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > >> 3 files changed, 214 insertions(+) > >> > >> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c > >> index fb3bbc77fd00..b0ac0ec2356d 100644 > >> --- a/tools/testing/selftests/mm/uffd-common.c > >> +++ b/tools/testing/selftests/mm/uffd-common.c > >> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > >> return __copy_page(ufd, offset, false, wp); > >> } > >> > >> +int move_page(int ufd, unsigned long offset, unsigned long len) > >> +{ > >> + struct uffdio_move uffdio_move; > >> + > >> + if (offset + len > nr_pages * page_size) > >> + err("unexpected offset %lu and length %lu\n", offset, len); > >> + uffdio_move.dst = (unsigned long) area_dst + offset; > >> + uffdio_move.src = (unsigned long) area_src + offset; > >> + uffdio_move.len = len; > >> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > >> + uffdio_move.move = 0; > >> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > >> + /* real retval in uffdio_move.move */ > >> + if (uffdio_move.move != -EEXIST) > >> + err("UFFDIO_MOVE error: %"PRId64, > >> + (int64_t)uffdio_move.move); > > > > Hi Suren, > > > > FYI this error is triggering in mm-unstable (715b67adf4c8): > > > > Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > > @uffd-common.c:648) > > > > I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > > happy to go deeper if you can direct. > > Does it trigger reliably? Which pagesize is that kernel using? > > I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > default_huge_page_size(), which reads the default hugetlb size. > > That, however, does not necessarily correspond to the THP size. That one > can be obtained using read_pmd_pagesize() in vm_util.c Oh, I didn't realize that default_huge_page_size() is not always the actual THP size. Will fix. > > I quickly scanned the code (still want to take a deeper look), but all > PAE checks looked sane to me. > > I think the issue is folio split handling. I replied to the patch. Thanks for your hint! That's very possibly the reason for the test failure. I'm in the process of trying to reproduce this issue on ARM64. If I'm unable to do so then I'll create a patch to split PTE-mapped THP when encountered and will ask Ryan to try that. Thanks, Suren. > > -- > Cheers, > > David / dhildenb >
On 01/12/2023 16:26, Suren Baghdasaryan wrote: > On Fri, Dec 1, 2023 at 1:29 AM Ryan Roberts <ryan.roberts@arm.com> wrote: >> >> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>> into destination buffer while checking the contents of both after >>> the move. After the operation the content of the destination buffer >>> should match the original source buffer's content while the source >>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>> unaligned cases because they utilize different code paths in the kernel. >>> >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>> --- >>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>> tools/testing/selftests/mm/uffd-common.h | 1 + >>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>> 3 files changed, 214 insertions(+) >>> >>> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c >>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>> --- a/tools/testing/selftests/mm/uffd-common.c >>> +++ b/tools/testing/selftests/mm/uffd-common.c >>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>> return __copy_page(ufd, offset, false, wp); >>> } >>> >>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>> +{ >>> + struct uffdio_move uffdio_move; >>> + >>> + if (offset + len > nr_pages * page_size) >>> + err("unexpected offset %lu and length %lu\n", offset, len); >>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>> + uffdio_move.src = (unsigned long) area_src + offset; >>> + uffdio_move.len = len; >>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>> + uffdio_move.move = 0; >>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>> + /* real retval in uffdio_move.move */ >>> + if (uffdio_move.move != -EEXIST) >>> + err("UFFDIO_MOVE error: %"PRId64, >>> + (int64_t)uffdio_move.move); >> >> Hi Suren, >> >> FYI this error is triggering in mm-unstable (715b67adf4c8): >> >> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >> @uffd-common.c:648) >> >> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >> happy to go deeper if you can direct. > > Hi Ryan, > Thanks for reporting! Could you please share your kernel config file? It's arm64 defconfig (so 4K base pages) plus: # Squashfs for snaps, xfs for large file folios. ./scripts/config --enable CONFIG_SQUASHFS_LZ4 ./scripts/config --enable CONFIG_SQUASHFS_LZO ./scripts/config --enable CONFIG_SQUASHFS_XZ ./scripts/config --enable CONFIG_SQUASHFS_ZSTD ./scripts/config --enable CONFIG_XFS_FS # Useful trace features (on for Ubuntu configs). ./scripts/config --enable CONFIG_FTRACE ./scripts/config --enable CONFIG_FUNCTION_TRACER ./scripts/config --enable CONFIG_KPROBES ./scripts/config --enable CONFIG_HIST_TRIGGERS ./scripts/config --enable CONFIG_FTRACE_SYSCALLS # For general mm debug. ./scripts/config --enable CONFIG_DEBUG_VM ./scripts/config --enable CONFIG_DEBUG_VM_MAPLE_TREE ./scripts/config --enable CONFIG_DEBUG_VM_RB ./scripts/config --enable CONFIG_DEBUG_VM_PGFLAGS ./scripts/config --enable CONFIG_DEBUG_VM_PGTABLE ./scripts/config --enable CONFIG_PAGE_TABLE_CHECK # For mm selftests. ./scripts/config --enable CONFIG_USERFAULTFD ./scripts/config --enable CONFIG_TEST_VMALLOC ./scripts/config --enable CONFIG_GUP_TEST This is the config I always use when running mm selftests. I'll send you the config file separately. Then I'm running in a QEMU VM with 12G RAM, equally split across 2 (emulated) numa nodes. I have these pertinent kernel command line args (intended to ensure all the mm selftests can run): transparent_hugepage=madvise secretmem.enable hugepagesz=1G hugepages=0:2,1:2 hugepagesz=32M hugepages=0:2,1:2 default_hugepagesz=2M hugepages=0:64,1:64 hugepagesz=64K hugepages=0:2,1:2 > > There are several places UFFDIO_MOVE returns EBUSY: 4 places in > move_pages_huge_pmd(), 2 places in move_present_pte(), 2 places in > move_pages_pte() and once in move_swap_pte(). While I'm trying to > reproduce, it would be useful if you could check which place is > triggering the error. Happy to, but will have to wait for Monday. I should say, the test fails consistently for me. But sometimes the failure is EAGAIN (11). Most of the time it is EBUSY though and I haven't figured out what causes the difference. > Thanks, > Suren. > >> >> Thanks, >> Ryan >> >> >>> + wake_range(ufd, uffdio_move.dst, len); >>> + } else if (uffdio_move.move != len) { >>> + err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move); >>> + } else >>> + return 1; >>> + return 0; >>> +} >> >> -- >> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >>
On 01/12/2023 20:47, David Hildenbrand wrote: > On 01.12.23 10:29, Ryan Roberts wrote: >> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>> into destination buffer while checking the contents of both after >>> the move. After the operation the content of the destination buffer >>> should match the original source buffer's content while the source >>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>> unaligned cases because they utilize different code paths in the kernel. >>> >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>> --- >>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>> tools/testing/selftests/mm/uffd-common.h | 1 + >>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>> 3 files changed, 214 insertions(+) >>> >>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>> b/tools/testing/selftests/mm/uffd-common.c >>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>> --- a/tools/testing/selftests/mm/uffd-common.c >>> +++ b/tools/testing/selftests/mm/uffd-common.c >>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>> return __copy_page(ufd, offset, false, wp); >>> } >>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>> +{ >>> + struct uffdio_move uffdio_move; >>> + >>> + if (offset + len > nr_pages * page_size) >>> + err("unexpected offset %lu and length %lu\n", offset, len); >>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>> + uffdio_move.src = (unsigned long) area_src + offset; >>> + uffdio_move.len = len; >>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>> + uffdio_move.move = 0; >>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>> + /* real retval in uffdio_move.move */ >>> + if (uffdio_move.move != -EEXIST) >>> + err("UFFDIO_MOVE error: %"PRId64, >>> + (int64_t)uffdio_move.move); >> >> Hi Suren, >> >> FYI this error is triggering in mm-unstable (715b67adf4c8): >> >> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >> @uffd-common.c:648) >> >> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >> happy to go deeper if you can direct. > > Does it trigger reliably? Which pagesize is that kernel using? Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config. > > I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > default_huge_page_size(), which reads the default hugetlb size. My kernel command line is explicitly seting the default huge page size to 2M. > > That, however, does not necessarily correspond to the THP size. That one can be > obtained using read_pmd_pagesize() in vm_util.c > > I quickly scanned the code (still want to take a deeper look), but all PAE > checks looked sane to me. > > I think the issue is folio split handling. I replied to the patch. >
On 02.12.23 09:04, Ryan Roberts wrote: > On 01/12/2023 20:47, David Hildenbrand wrote: >> On 01.12.23 10:29, Ryan Roberts wrote: >>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>> into destination buffer while checking the contents of both after >>>> the move. After the operation the content of the destination buffer >>>> should match the original source buffer's content while the source >>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>> unaligned cases because they utilize different code paths in the kernel. >>>> >>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>>> --- >>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>> 3 files changed, 214 insertions(+) >>>> >>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>> b/tools/testing/selftests/mm/uffd-common.c >>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>> return __copy_page(ufd, offset, false, wp); >>>> } >>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>> +{ >>>> + struct uffdio_move uffdio_move; >>>> + >>>> + if (offset + len > nr_pages * page_size) >>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>> + uffdio_move.len = len; >>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>> + uffdio_move.move = 0; >>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>> + /* real retval in uffdio_move.move */ >>>> + if (uffdio_move.move != -EEXIST) >>>> + err("UFFDIO_MOVE error: %"PRId64, >>>> + (int64_t)uffdio_move.move); >>> >>> Hi Suren, >>> >>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>> >>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>> @uffd-common.c:648) >>> >>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>> happy to go deeper if you can direct. >> >> Does it trigger reliably? Which pagesize is that kernel using? > > Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > for full config. > >> >> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >> default_huge_page_size(), which reads the default hugetlb size. > > My kernel command line is explicitly seting the default huge page size to 2M. > Okay, so that likely won't affect it. I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64. uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified? So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest? Not sure if that could trigger the THP splitting issue, though. But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: > > On 02.12.23 09:04, Ryan Roberts wrote: > > On 01/12/2023 20:47, David Hildenbrand wrote: > >> On 01.12.23 10:29, Ryan Roberts wrote: > >>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: > >>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > >>>> into destination buffer while checking the contents of both after > >>>> the move. After the operation the content of the destination buffer > >>>> should match the original source buffer's content while the source > >>>> buffer should be zeroed. Separate tests are designed for PMD aligned and > >>>> unaligned cases because they utilize different code paths in the kernel. > >>>> > >>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >>>> --- > >>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ > >>>> tools/testing/selftests/mm/uffd-common.h | 1 + > >>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > >>>> 3 files changed, 214 insertions(+) > >>>> > >>>> diff --git a/tools/testing/selftests/mm/uffd-common.c > >>>> b/tools/testing/selftests/mm/uffd-common.c > >>>> index fb3bbc77fd00..b0ac0ec2356d 100644 > >>>> --- a/tools/testing/selftests/mm/uffd-common.c > >>>> +++ b/tools/testing/selftests/mm/uffd-common.c > >>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > >>>> return __copy_page(ufd, offset, false, wp); > >>>> } > >>>> +int move_page(int ufd, unsigned long offset, unsigned long len) > >>>> +{ > >>>> + struct uffdio_move uffdio_move; > >>>> + > >>>> + if (offset + len > nr_pages * page_size) > >>>> + err("unexpected offset %lu and length %lu\n", offset, len); > >>>> + uffdio_move.dst = (unsigned long) area_dst + offset; > >>>> + uffdio_move.src = (unsigned long) area_src + offset; > >>>> + uffdio_move.len = len; > >>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > >>>> + uffdio_move.move = 0; > >>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > >>>> + /* real retval in uffdio_move.move */ > >>>> + if (uffdio_move.move != -EEXIST) > >>>> + err("UFFDIO_MOVE error: %"PRId64, > >>>> + (int64_t)uffdio_move.move); > >>> > >>> Hi Suren, > >>> > >>> FYI this error is triggering in mm-unstable (715b67adf4c8): > >>> > >>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > >>> @uffd-common.c:648) > >>> > >>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > >>> happy to go deeper if you can direct. > >> > >> Does it trigger reliably? Which pagesize is that kernel using? > > > > Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > > for full config. > > > >> > >> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > >> default_huge_page_size(), which reads the default hugetlb size. > > > > My kernel command line is explicitly seting the default huge page size to 2M. > > > > Okay, so that likely won't affect it. > > I can only guess that it has to do with the alignment of the virtual > area we are testing with, and that we do seem to get more odd patterns > on arm64. > > uffd_move_test_common() is a bit more elaborate, but if we aligned the > src+start area up, surely "step_count" cannot be left unmodified? > > So assuming we get either an unaligned source or an unaligned dst from > mmap(), I am not convinced that we won't be moving areas that are not > necessarily fully backed by PMDs and maybe don't even fall into the VMA > of interest? > > Not sure if that could trigger the THP splitting issue, though. > > But I just quickly scanned that test setup, could be I am missing > something. It might make sense to just print the mmap'ed range and the > actual ranges we are trying to move. Maybe something "obvious" can be > observed. I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again? Thanks, Suren. > > -- > Cheers, > > David / dhildenb >
On 04/12/2023 04:09, Suren Baghdasaryan wrote: > On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: >> >> On 02.12.23 09:04, Ryan Roberts wrote: >>> On 01/12/2023 20:47, David Hildenbrand wrote: >>>> On 01.12.23 10:29, Ryan Roberts wrote: >>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>>>> into destination buffer while checking the contents of both after >>>>>> the move. After the operation the content of the destination buffer >>>>>> should match the original source buffer's content while the source >>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>>>> unaligned cases because they utilize different code paths in the kernel. >>>>>> >>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>>>>> --- >>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>>>> 3 files changed, 214 insertions(+) >>>>>> >>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>>>> b/tools/testing/selftests/mm/uffd-common.c >>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>>>> return __copy_page(ufd, offset, false, wp); >>>>>> } >>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>>>> +{ >>>>>> + struct uffdio_move uffdio_move; >>>>>> + >>>>>> + if (offset + len > nr_pages * page_size) >>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>>>> + uffdio_move.len = len; >>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>>>> + uffdio_move.move = 0; >>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>>>> + /* real retval in uffdio_move.move */ >>>>>> + if (uffdio_move.move != -EEXIST) >>>>>> + err("UFFDIO_MOVE error: %"PRId64, >>>>>> + (int64_t)uffdio_move.move); >>>>> >>>>> Hi Suren, >>>>> >>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>>>> >>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>>>> @uffd-common.c:648) >>>>> >>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>>>> happy to go deeper if you can direct. >>>> >>>> Does it trigger reliably? Which pagesize is that kernel using? >>> >>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email >>> for full config. >>> >>>> >>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >>>> default_huge_page_size(), which reads the default hugetlb size. >>> >>> My kernel command line is explicitly seting the default huge page size to 2M. >>> >> >> Okay, so that likely won't affect it. >> >> I can only guess that it has to do with the alignment of the virtual >> area we are testing with, and that we do seem to get more odd patterns >> on arm64. >> >> uffd_move_test_common() is a bit more elaborate, but if we aligned the >> src+start area up, surely "step_count" cannot be left unmodified? >> >> So assuming we get either an unaligned source or an unaligned dst from >> mmap(), I am not convinced that we won't be moving areas that are not >> necessarily fully backed by PMDs and maybe don't even fall into the VMA >> of interest? >> >> Not sure if that could trigger the THP splitting issue, though. >> >> But I just quickly scanned that test setup, could be I am missing >> something. It might make sense to just print the mmap'ed range and the >> actual ranges we are trying to move. Maybe something "obvious" can be >> observed. > > I was able to reproduce the issue on an Android device and after > implementing David's suggestions to split the large folio and after > replacing default_huge_page_size() with read_pmd_pagesize(), the > move-pmd test started working for me. Ryan, could you please apply > attached patches (over mm-unstable) and try the test again? Yep, all fixed with those patches! > Thanks, > Suren. > >> >> -- >> Cheers, >> >> David / dhildenb >>
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > > On 04/12/2023 04:09, Suren Baghdasaryan wrote: > > On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: > >> > >> On 02.12.23 09:04, Ryan Roberts wrote: > >>> On 01/12/2023 20:47, David Hildenbrand wrote: > >>>> On 01.12.23 10:29, Ryan Roberts wrote: > >>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: > >>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > >>>>>> into destination buffer while checking the contents of both after > >>>>>> the move. After the operation the content of the destination buffer > >>>>>> should match the original source buffer's content while the source > >>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and > >>>>>> unaligned cases because they utilize different code paths in the kernel. > >>>>>> > >>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >>>>>> --- > >>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ > >>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + > >>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > >>>>>> 3 files changed, 214 insertions(+) > >>>>>> > >>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c > >>>>>> b/tools/testing/selftests/mm/uffd-common.c > >>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 > >>>>>> --- a/tools/testing/selftests/mm/uffd-common.c > >>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c > >>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > >>>>>> return __copy_page(ufd, offset, false, wp); > >>>>>> } > >>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) > >>>>>> +{ > >>>>>> + struct uffdio_move uffdio_move; > >>>>>> + > >>>>>> + if (offset + len > nr_pages * page_size) > >>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); > >>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; > >>>>>> + uffdio_move.src = (unsigned long) area_src + offset; > >>>>>> + uffdio_move.len = len; > >>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > >>>>>> + uffdio_move.move = 0; > >>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > >>>>>> + /* real retval in uffdio_move.move */ > >>>>>> + if (uffdio_move.move != -EEXIST) > >>>>>> + err("UFFDIO_MOVE error: %"PRId64, > >>>>>> + (int64_t)uffdio_move.move); > >>>>> > >>>>> Hi Suren, > >>>>> > >>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): > >>>>> > >>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > >>>>> @uffd-common.c:648) > >>>>> > >>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > >>>>> happy to go deeper if you can direct. > >>>> > >>>> Does it trigger reliably? Which pagesize is that kernel using? > >>> > >>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > >>> for full config. > >>> > >>>> > >>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > >>>> default_huge_page_size(), which reads the default hugetlb size. > >>> > >>> My kernel command line is explicitly seting the default huge page size to 2M. > >>> > >> > >> Okay, so that likely won't affect it. > >> > >> I can only guess that it has to do with the alignment of the virtual > >> area we are testing with, and that we do seem to get more odd patterns > >> on arm64. > >> > >> uffd_move_test_common() is a bit more elaborate, but if we aligned the > >> src+start area up, surely "step_count" cannot be left unmodified? > >> > >> So assuming we get either an unaligned source or an unaligned dst from > >> mmap(), I am not convinced that we won't be moving areas that are not > >> necessarily fully backed by PMDs and maybe don't even fall into the VMA > >> of interest? > >> > >> Not sure if that could trigger the THP splitting issue, though. > >> > >> But I just quickly scanned that test setup, could be I am missing > >> something. It might make sense to just print the mmap'ed range and the > >> actual ranges we are trying to move. Maybe something "obvious" can be > >> observed. > > > > I was able to reproduce the issue on an Android device and after > > implementing David's suggestions to split the large folio and after > > replacing default_huge_page_size() with read_pmd_pagesize(), the > > move-pmd test started working for me. Ryan, could you please apply > > attached patches (over mm-unstable) and try the test again? > > Yep, all fixed with those patches! Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect. Thanks, Suren. > > > > Thanks, > > Suren. > > > >> > >> -- > >> Cheers, > >> > >> David / dhildenb > >> >
On 04.12.23 17:35, Suren Baghdasaryan wrote: > On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts <ryan.roberts@arm.com> wrote: >> >> On 04/12/2023 04:09, Suren Baghdasaryan wrote: >>> On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: >>>> >>>> On 02.12.23 09:04, Ryan Roberts wrote: >>>>> On 01/12/2023 20:47, David Hildenbrand wrote: >>>>>> On 01.12.23 10:29, Ryan Roberts wrote: >>>>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>>>>>> into destination buffer while checking the contents of both after >>>>>>>> the move. After the operation the content of the destination buffer >>>>>>>> should match the original source buffer's content while the source >>>>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>>>>>> unaligned cases because they utilize different code paths in the kernel. >>>>>>>> >>>>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>>>>>>> --- >>>>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>>>>>> 3 files changed, 214 insertions(+) >>>>>>>> >>>>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>>>>>> b/tools/testing/selftests/mm/uffd-common.c >>>>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>>>>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>>>>>> return __copy_page(ufd, offset, false, wp); >>>>>>>> } >>>>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>>>>>> +{ >>>>>>>> + struct uffdio_move uffdio_move; >>>>>>>> + >>>>>>>> + if (offset + len > nr_pages * page_size) >>>>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>>>>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>>>>>> + uffdio_move.len = len; >>>>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>>>>>> + uffdio_move.move = 0; >>>>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>>>>>> + /* real retval in uffdio_move.move */ >>>>>>>> + if (uffdio_move.move != -EEXIST) >>>>>>>> + err("UFFDIO_MOVE error: %"PRId64, >>>>>>>> + (int64_t)uffdio_move.move); >>>>>>> >>>>>>> Hi Suren, >>>>>>> >>>>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>>>>>> >>>>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>>>>>> @uffd-common.c:648) >>>>>>> >>>>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>>>>>> happy to go deeper if you can direct. >>>>>> >>>>>> Does it trigger reliably? Which pagesize is that kernel using? >>>>> >>>>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email >>>>> for full config. >>>>> >>>>>> >>>>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >>>>>> default_huge_page_size(), which reads the default hugetlb size. >>>>> >>>>> My kernel command line is explicitly seting the default huge page size to 2M. >>>>> >>>> >>>> Okay, so that likely won't affect it. >>>> >>>> I can only guess that it has to do with the alignment of the virtual >>>> area we are testing with, and that we do seem to get more odd patterns >>>> on arm64. >>>> >>>> uffd_move_test_common() is a bit more elaborate, but if we aligned the >>>> src+start area up, surely "step_count" cannot be left unmodified? >>>> >>>> So assuming we get either an unaligned source or an unaligned dst from >>>> mmap(), I am not convinced that we won't be moving areas that are not >>>> necessarily fully backed by PMDs and maybe don't even fall into the VMA >>>> of interest? >>>> >>>> Not sure if that could trigger the THP splitting issue, though. >>>> >>>> But I just quickly scanned that test setup, could be I am missing >>>> something. It might make sense to just print the mmap'ed range and the >>>> actual ranges we are trying to move. Maybe something "obvious" can be >>>> observed. >>> >>> I was able to reproduce the issue on an Android device and after >>> implementing David's suggestions to split the large folio and after >>> replacing default_huge_page_size() with read_pmd_pagesize(), the >>> move-pmd test started working for me. Ryan, could you please apply >>> attached patches (over mm-unstable) and try the test again? >> >> Yep, all fixed with those patches! > > Great! Thanks for testing and confirming. I'll post an updated > patchset later today and will ask Andrew to replace the current one > with it. > I'll also look into the reasons we need to split PMD on ARM64 in this > test. It's good that this happened and we were able to test the PMD > split path but I'm curious about the reason. It's possible my address > alignment calculations are somehow incorrect. I only skimmed the diff briefly, but likely you also want to try splitting in move_pages_pte(), if you encounter an already-pte-mapped THP.
On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand <david@redhat.com> wrote: > > On 04.12.23 17:35, Suren Baghdasaryan wrote: > > On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > >> > >> On 04/12/2023 04:09, Suren Baghdasaryan wrote: > >>> On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: > >>>> > >>>> On 02.12.23 09:04, Ryan Roberts wrote: > >>>>> On 01/12/2023 20:47, David Hildenbrand wrote: > >>>>>> On 01.12.23 10:29, Ryan Roberts wrote: > >>>>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: > >>>>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > >>>>>>>> into destination buffer while checking the contents of both after > >>>>>>>> the move. After the operation the content of the destination buffer > >>>>>>>> should match the original source buffer's content while the source > >>>>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and > >>>>>>>> unaligned cases because they utilize different code paths in the kernel. > >>>>>>>> > >>>>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >>>>>>>> --- > >>>>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ > >>>>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + > >>>>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > >>>>>>>> 3 files changed, 214 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c > >>>>>>>> b/tools/testing/selftests/mm/uffd-common.c > >>>>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 > >>>>>>>> --- a/tools/testing/selftests/mm/uffd-common.c > >>>>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c > >>>>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > >>>>>>>> return __copy_page(ufd, offset, false, wp); > >>>>>>>> } > >>>>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) > >>>>>>>> +{ > >>>>>>>> + struct uffdio_move uffdio_move; > >>>>>>>> + > >>>>>>>> + if (offset + len > nr_pages * page_size) > >>>>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); > >>>>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; > >>>>>>>> + uffdio_move.src = (unsigned long) area_src + offset; > >>>>>>>> + uffdio_move.len = len; > >>>>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > >>>>>>>> + uffdio_move.move = 0; > >>>>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > >>>>>>>> + /* real retval in uffdio_move.move */ > >>>>>>>> + if (uffdio_move.move != -EEXIST) > >>>>>>>> + err("UFFDIO_MOVE error: %"PRId64, > >>>>>>>> + (int64_t)uffdio_move.move); > >>>>>>> > >>>>>>> Hi Suren, > >>>>>>> > >>>>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): > >>>>>>> > >>>>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > >>>>>>> @uffd-common.c:648) > >>>>>>> > >>>>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > >>>>>>> happy to go deeper if you can direct. > >>>>>> > >>>>>> Does it trigger reliably? Which pagesize is that kernel using? > >>>>> > >>>>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > >>>>> for full config. > >>>>> > >>>>>> > >>>>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > >>>>>> default_huge_page_size(), which reads the default hugetlb size. > >>>>> > >>>>> My kernel command line is explicitly seting the default huge page size to 2M. > >>>>> > >>>> > >>>> Okay, so that likely won't affect it. > >>>> > >>>> I can only guess that it has to do with the alignment of the virtual > >>>> area we are testing with, and that we do seem to get more odd patterns > >>>> on arm64. > >>>> > >>>> uffd_move_test_common() is a bit more elaborate, but if we aligned the > >>>> src+start area up, surely "step_count" cannot be left unmodified? > >>>> > >>>> So assuming we get either an unaligned source or an unaligned dst from > >>>> mmap(), I am not convinced that we won't be moving areas that are not > >>>> necessarily fully backed by PMDs and maybe don't even fall into the VMA > >>>> of interest? > >>>> > >>>> Not sure if that could trigger the THP splitting issue, though. > >>>> > >>>> But I just quickly scanned that test setup, could be I am missing > >>>> something. It might make sense to just print the mmap'ed range and the > >>>> actual ranges we are trying to move. Maybe something "obvious" can be > >>>> observed. > >>> > >>> I was able to reproduce the issue on an Android device and after > >>> implementing David's suggestions to split the large folio and after > >>> replacing default_huge_page_size() with read_pmd_pagesize(), the > >>> move-pmd test started working for me. Ryan, could you please apply > >>> attached patches (over mm-unstable) and try the test again? > >> > >> Yep, all fixed with those patches! > > > > Great! Thanks for testing and confirming. I'll post an updated > > patchset later today and will ask Andrew to replace the current one > > with it. > > I'll also look into the reasons we need to split PMD on ARM64 in this > > test. It's good that this happened and we were able to test the PMD > > split path but I'm curious about the reason. It's possible my address > > alignment calculations are somehow incorrect. > > I only skimmed the diff briefly, but likely you also want to try > splitting in move_pages_pte(), if you encounter an already-pte-mapped THP. Huh, good point. I might be able to move the folio splitting code into pte-mapped case and do a retry after splitting. That should minimize the additional code required. Will do and post a new set shortly. Thanks! > > -- > Cheers, > > David / dhildenb >
On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan <surenb@google.com> wrote: > > On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand <david@redhat.com> wrote: > > > > On 04.12.23 17:35, Suren Baghdasaryan wrote: > > > On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > > >> > > >> On 04/12/2023 04:09, Suren Baghdasaryan wrote: > > >>> On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: > > >>>> > > >>>> On 02.12.23 09:04, Ryan Roberts wrote: > > >>>>> On 01/12/2023 20:47, David Hildenbrand wrote: > > >>>>>> On 01.12.23 10:29, Ryan Roberts wrote: > > >>>>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: > > >>>>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > > >>>>>>>> into destination buffer while checking the contents of both after > > >>>>>>>> the move. After the operation the content of the destination buffer > > >>>>>>>> should match the original source buffer's content while the source > > >>>>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and > > >>>>>>>> unaligned cases because they utilize different code paths in the kernel. > > >>>>>>>> > > >>>>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > >>>>>>>> --- > > >>>>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ > > >>>>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + > > >>>>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > > >>>>>>>> 3 files changed, 214 insertions(+) > > >>>>>>>> > > >>>>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>> b/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 > > >>>>>>>> --- a/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > > >>>>>>>> return __copy_page(ufd, offset, false, wp); > > >>>>>>>> } > > >>>>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) > > >>>>>>>> +{ > > >>>>>>>> + struct uffdio_move uffdio_move; > > >>>>>>>> + > > >>>>>>>> + if (offset + len > nr_pages * page_size) > > >>>>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); > > >>>>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; > > >>>>>>>> + uffdio_move.src = (unsigned long) area_src + offset; > > >>>>>>>> + uffdio_move.len = len; > > >>>>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > > >>>>>>>> + uffdio_move.move = 0; > > >>>>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > > >>>>>>>> + /* real retval in uffdio_move.move */ > > >>>>>>>> + if (uffdio_move.move != -EEXIST) > > >>>>>>>> + err("UFFDIO_MOVE error: %"PRId64, > > >>>>>>>> + (int64_t)uffdio_move.move); > > >>>>>>> > > >>>>>>> Hi Suren, > > >>>>>>> > > >>>>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): > > >>>>>>> > > >>>>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > > >>>>>>> @uffd-common.c:648) > > >>>>>>> > > >>>>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > > >>>>>>> happy to go deeper if you can direct. > > >>>>>> > > >>>>>> Does it trigger reliably? Which pagesize is that kernel using? > > >>>>> > > >>>>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > > >>>>> for full config. > > >>>>> > > >>>>>> > > >>>>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > > >>>>>> default_huge_page_size(), which reads the default hugetlb size. > > >>>>> > > >>>>> My kernel command line is explicitly seting the default huge page size to 2M. > > >>>>> > > >>>> > > >>>> Okay, so that likely won't affect it. > > >>>> > > >>>> I can only guess that it has to do with the alignment of the virtual > > >>>> area we are testing with, and that we do seem to get more odd patterns > > >>>> on arm64. > > >>>> > > >>>> uffd_move_test_common() is a bit more elaborate, but if we aligned the > > >>>> src+start area up, surely "step_count" cannot be left unmodified? > > >>>> > > >>>> So assuming we get either an unaligned source or an unaligned dst from > > >>>> mmap(), I am not convinced that we won't be moving areas that are not > > >>>> necessarily fully backed by PMDs and maybe don't even fall into the VMA > > >>>> of interest? > > >>>> > > >>>> Not sure if that could trigger the THP splitting issue, though. > > >>>> > > >>>> But I just quickly scanned that test setup, could be I am missing > > >>>> something. It might make sense to just print the mmap'ed range and the > > >>>> actual ranges we are trying to move. Maybe something "obvious" can be > > >>>> observed. > > >>> > > >>> I was able to reproduce the issue on an Android device and after > > >>> implementing David's suggestions to split the large folio and after > > >>> replacing default_huge_page_size() with read_pmd_pagesize(), the > > >>> move-pmd test started working for me. Ryan, could you please apply > > >>> attached patches (over mm-unstable) and try the test again? > > >> > > >> Yep, all fixed with those patches! > > > > > > Great! Thanks for testing and confirming. I'll post an updated > > > patchset later today and will ask Andrew to replace the current one > > > with it. > > > I'll also look into the reasons we need to split PMD on ARM64 in this > > > test. It's good that this happened and we were able to test the PMD > > > split path but I'm curious about the reason. It's possible my address > > > alignment calculations are somehow incorrect. > > > > I only skimmed the diff briefly, but likely you also want to try > > splitting in move_pages_pte(), if you encounter an already-pte-mapped THP. > > Huh, good point. I might be able to move the folio splitting code into > pte-mapped case and do a retry after splitting. That should minimize > the additional code required. Will do and post a new set shortly. > Thanks! Was planning to post an update today but need some more time. Will try to send it tomorrow. > > > > > -- > > Cheers, > > > > David / dhildenb > >
On 05.12.23 05:46, Suren Baghdasaryan wrote: > On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan <surenb@google.com> wrote: >> >> On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand <david@redhat.com> wrote: >>> >>> On 04.12.23 17:35, Suren Baghdasaryan wrote: >>>> On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts <ryan.roberts@arm.com> wrote: >>>>> >>>>> On 04/12/2023 04:09, Suren Baghdasaryan wrote: >>>>>> On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: >>>>>>> >>>>>>> On 02.12.23 09:04, Ryan Roberts wrote: >>>>>>>> On 01/12/2023 20:47, David Hildenbrand wrote: >>>>>>>>> On 01.12.23 10:29, Ryan Roberts wrote: >>>>>>>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>>>>>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>>>>>>>>> into destination buffer while checking the contents of both after >>>>>>>>>>> the move. After the operation the content of the destination buffer >>>>>>>>>>> should match the original source buffer's content while the source >>>>>>>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>>>>>>>>> unaligned cases because they utilize different code paths in the kernel. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>>>>>>>>>> --- >>>>>>>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>>>>>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>>>>>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>>>>>>>>> 3 files changed, 214 insertions(+) >>>>>>>>>>> >>>>>>>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>>>>>>>>> b/tools/testing/selftests/mm/uffd-common.c >>>>>>>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>>>>>>>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>>>>>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>>>>>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>>>>>>>>> return __copy_page(ufd, offset, false, wp); >>>>>>>>>>> } >>>>>>>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>>>>>>>>> +{ >>>>>>>>>>> + struct uffdio_move uffdio_move; >>>>>>>>>>> + >>>>>>>>>>> + if (offset + len > nr_pages * page_size) >>>>>>>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>>>>>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>>>>>>>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>>>>>>>>> + uffdio_move.len = len; >>>>>>>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>>>>>>>>> + uffdio_move.move = 0; >>>>>>>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>>>>>>>>> + /* real retval in uffdio_move.move */ >>>>>>>>>>> + if (uffdio_move.move != -EEXIST) >>>>>>>>>>> + err("UFFDIO_MOVE error: %"PRId64, >>>>>>>>>>> + (int64_t)uffdio_move.move); >>>>>>>>>> >>>>>>>>>> Hi Suren, >>>>>>>>>> >>>>>>>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>>>>>>>>> >>>>>>>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>>>>>>>>> @uffd-common.c:648) >>>>>>>>>> >>>>>>>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>>>>>>>>> happy to go deeper if you can direct. >>>>>>>>> >>>>>>>>> Does it trigger reliably? Which pagesize is that kernel using? >>>>>>>> >>>>>>>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email >>>>>>>> for full config. >>>>>>>> >>>>>>>>> >>>>>>>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >>>>>>>>> default_huge_page_size(), which reads the default hugetlb size. >>>>>>>> >>>>>>>> My kernel command line is explicitly seting the default huge page size to 2M. >>>>>>>> >>>>>>> >>>>>>> Okay, so that likely won't affect it. >>>>>>> >>>>>>> I can only guess that it has to do with the alignment of the virtual >>>>>>> area we are testing with, and that we do seem to get more odd patterns >>>>>>> on arm64. >>>>>>> >>>>>>> uffd_move_test_common() is a bit more elaborate, but if we aligned the >>>>>>> src+start area up, surely "step_count" cannot be left unmodified? >>>>>>> >>>>>>> So assuming we get either an unaligned source or an unaligned dst from >>>>>>> mmap(), I am not convinced that we won't be moving areas that are not >>>>>>> necessarily fully backed by PMDs and maybe don't even fall into the VMA >>>>>>> of interest? >>>>>>> >>>>>>> Not sure if that could trigger the THP splitting issue, though. >>>>>>> >>>>>>> But I just quickly scanned that test setup, could be I am missing >>>>>>> something. It might make sense to just print the mmap'ed range and the >>>>>>> actual ranges we are trying to move. Maybe something "obvious" can be >>>>>>> observed. >>>>>> >>>>>> I was able to reproduce the issue on an Android device and after >>>>>> implementing David's suggestions to split the large folio and after >>>>>> replacing default_huge_page_size() with read_pmd_pagesize(), the >>>>>> move-pmd test started working for me. Ryan, could you please apply >>>>>> attached patches (over mm-unstable) and try the test again? >>>>> >>>>> Yep, all fixed with those patches! >>>> >>>> Great! Thanks for testing and confirming. I'll post an updated >>>> patchset later today and will ask Andrew to replace the current one >>>> with it. >>>> I'll also look into the reasons we need to split PMD on ARM64 in this >>>> test. It's good that this happened and we were able to test the PMD >>>> split path but I'm curious about the reason. It's possible my address >>>> alignment calculations are somehow incorrect. >>> >>> I only skimmed the diff briefly, but likely you also want to try >>> splitting in move_pages_pte(), if you encounter an already-pte-mapped THP. >> >> Huh, good point. I might be able to move the folio splitting code into >> pte-mapped case and do a retry after splitting. That should minimize >> the additional code required. Will do and post a new set shortly. >> Thanks! > > Was planning to post an update today but need some more time. Will try > to send it tomorrow. It would be great to have tests that cover these cases (having to PTE-map a PMD-mapped THP, and stumbling over an already-PTE-mapped one).
On Wed, Dec 6, 2023 at 1:21 AM David Hildenbrand <david@redhat.com> wrote: > > On 05.12.23 05:46, Suren Baghdasaryan wrote: > > On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan <surenb@google.com> wrote: > >> > >> On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand <david@redhat.com> wrote: > >>> > >>> On 04.12.23 17:35, Suren Baghdasaryan wrote: > >>>> On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > >>>>> > >>>>> On 04/12/2023 04:09, Suren Baghdasaryan wrote: > >>>>>> On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: > >>>>>>> > >>>>>>> On 02.12.23 09:04, Ryan Roberts wrote: > >>>>>>>> On 01/12/2023 20:47, David Hildenbrand wrote: > >>>>>>>>> On 01.12.23 10:29, Ryan Roberts wrote: > >>>>>>>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: > >>>>>>>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > >>>>>>>>>>> into destination buffer while checking the contents of both after > >>>>>>>>>>> the move. After the operation the content of the destination buffer > >>>>>>>>>>> should match the original source buffer's content while the source > >>>>>>>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and > >>>>>>>>>>> unaligned cases because they utilize different code paths in the kernel. > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >>>>>>>>>>> --- > >>>>>>>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ > >>>>>>>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + > >>>>>>>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > >>>>>>>>>>> 3 files changed, 214 insertions(+) > >>>>>>>>>>> > >>>>>>>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c > >>>>>>>>>>> b/tools/testing/selftests/mm/uffd-common.c > >>>>>>>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 > >>>>>>>>>>> --- a/tools/testing/selftests/mm/uffd-common.c > >>>>>>>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c > >>>>>>>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > >>>>>>>>>>> return __copy_page(ufd, offset, false, wp); > >>>>>>>>>>> } > >>>>>>>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) > >>>>>>>>>>> +{ > >>>>>>>>>>> + struct uffdio_move uffdio_move; > >>>>>>>>>>> + > >>>>>>>>>>> + if (offset + len > nr_pages * page_size) > >>>>>>>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); > >>>>>>>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; > >>>>>>>>>>> + uffdio_move.src = (unsigned long) area_src + offset; > >>>>>>>>>>> + uffdio_move.len = len; > >>>>>>>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > >>>>>>>>>>> + uffdio_move.move = 0; > >>>>>>>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > >>>>>>>>>>> + /* real retval in uffdio_move.move */ > >>>>>>>>>>> + if (uffdio_move.move != -EEXIST) > >>>>>>>>>>> + err("UFFDIO_MOVE error: %"PRId64, > >>>>>>>>>>> + (int64_t)uffdio_move.move); > >>>>>>>>>> > >>>>>>>>>> Hi Suren, > >>>>>>>>>> > >>>>>>>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): > >>>>>>>>>> > >>>>>>>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > >>>>>>>>>> @uffd-common.c:648) > >>>>>>>>>> > >>>>>>>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > >>>>>>>>>> happy to go deeper if you can direct. > >>>>>>>>> > >>>>>>>>> Does it trigger reliably? Which pagesize is that kernel using? > >>>>>>>> > >>>>>>>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > >>>>>>>> for full config. > >>>>>>>> > >>>>>>>>> > >>>>>>>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > >>>>>>>>> default_huge_page_size(), which reads the default hugetlb size. > >>>>>>>> > >>>>>>>> My kernel command line is explicitly seting the default huge page size to 2M. > >>>>>>>> > >>>>>>> > >>>>>>> Okay, so that likely won't affect it. > >>>>>>> > >>>>>>> I can only guess that it has to do with the alignment of the virtual > >>>>>>> area we are testing with, and that we do seem to get more odd patterns > >>>>>>> on arm64. > >>>>>>> > >>>>>>> uffd_move_test_common() is a bit more elaborate, but if we aligned the > >>>>>>> src+start area up, surely "step_count" cannot be left unmodified? > >>>>>>> > >>>>>>> So assuming we get either an unaligned source or an unaligned dst from > >>>>>>> mmap(), I am not convinced that we won't be moving areas that are not > >>>>>>> necessarily fully backed by PMDs and maybe don't even fall into the VMA > >>>>>>> of interest? > >>>>>>> > >>>>>>> Not sure if that could trigger the THP splitting issue, though. > >>>>>>> > >>>>>>> But I just quickly scanned that test setup, could be I am missing > >>>>>>> something. It might make sense to just print the mmap'ed range and the > >>>>>>> actual ranges we are trying to move. Maybe something "obvious" can be > >>>>>>> observed. > >>>>>> > >>>>>> I was able to reproduce the issue on an Android device and after > >>>>>> implementing David's suggestions to split the large folio and after > >>>>>> replacing default_huge_page_size() with read_pmd_pagesize(), the > >>>>>> move-pmd test started working for me. Ryan, could you please apply > >>>>>> attached patches (over mm-unstable) and try the test again? > >>>>> > >>>>> Yep, all fixed with those patches! > >>>> > >>>> Great! Thanks for testing and confirming. I'll post an updated > >>>> patchset later today and will ask Andrew to replace the current one > >>>> with it. > >>>> I'll also look into the reasons we need to split PMD on ARM64 in this > >>>> test. It's good that this happened and we were able to test the PMD > >>>> split path but I'm curious about the reason. It's possible my address > >>>> alignment calculations are somehow incorrect. > >>> > >>> I only skimmed the diff briefly, but likely you also want to try > >>> splitting in move_pages_pte(), if you encounter an already-pte-mapped THP. > >> > >> Huh, good point. I might be able to move the folio splitting code into > >> pte-mapped case and do a retry after splitting. That should minimize > >> the additional code required. Will do and post a new set shortly. > >> Thanks! > > > > Was planning to post an update today but need some more time. Will try > > to send it tomorrow. > > It would be great to have tests that cover these cases (having to > PTE-map a PMD-mapped THP, and stumbling over an already-PTE-mapped one). Agree. Let me post the new version so that mm-unstable does not produce these failures and will start working on covering additional cases in the tests. The new patchset is almost ready, just finishing final tests. > > -- > Cheers, > > David / dhildenb >
On Wed, Dec 6, 2023 at 2:30 AM Suren Baghdasaryan <surenb@google.com> wrote: > > On Wed, Dec 6, 2023 at 1:21 AM David Hildenbrand <david@redhat.com> wrote: > > > > On 05.12.23 05:46, Suren Baghdasaryan wrote: > > > On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan <surenb@google.com> wrote: > > >> > > >> On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand <david@redhat.com> wrote: > > >>> > > >>> On 04.12.23 17:35, Suren Baghdasaryan wrote: > > >>>> On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > > >>>>> > > >>>>> On 04/12/2023 04:09, Suren Baghdasaryan wrote: > > >>>>>> On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand <david@redhat.com> wrote: > > >>>>>>> > > >>>>>>> On 02.12.23 09:04, Ryan Roberts wrote: > > >>>>>>>> On 01/12/2023 20:47, David Hildenbrand wrote: > > >>>>>>>>> On 01.12.23 10:29, Ryan Roberts wrote: > > >>>>>>>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: > > >>>>>>>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > > >>>>>>>>>>> into destination buffer while checking the contents of both after > > >>>>>>>>>>> the move. After the operation the content of the destination buffer > > >>>>>>>>>>> should match the original source buffer's content while the source > > >>>>>>>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and > > >>>>>>>>>>> unaligned cases because they utilize different code paths in the kernel. > > >>>>>>>>>>> > > >>>>>>>>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > >>>>>>>>>>> --- > > >>>>>>>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ > > >>>>>>>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + > > >>>>>>>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > > >>>>>>>>>>> 3 files changed, 214 insertions(+) > > >>>>>>>>>>> > > >>>>>>>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>>>>> b/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 > > >>>>>>>>>>> --- a/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c > > >>>>>>>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > > >>>>>>>>>>> return __copy_page(ufd, offset, false, wp); > > >>>>>>>>>>> } > > >>>>>>>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) > > >>>>>>>>>>> +{ > > >>>>>>>>>>> + struct uffdio_move uffdio_move; > > >>>>>>>>>>> + > > >>>>>>>>>>> + if (offset + len > nr_pages * page_size) > > >>>>>>>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); > > >>>>>>>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; > > >>>>>>>>>>> + uffdio_move.src = (unsigned long) area_src + offset; > > >>>>>>>>>>> + uffdio_move.len = len; > > >>>>>>>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > > >>>>>>>>>>> + uffdio_move.move = 0; > > >>>>>>>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > > >>>>>>>>>>> + /* real retval in uffdio_move.move */ > > >>>>>>>>>>> + if (uffdio_move.move != -EEXIST) > > >>>>>>>>>>> + err("UFFDIO_MOVE error: %"PRId64, > > >>>>>>>>>>> + (int64_t)uffdio_move.move); > > >>>>>>>>>> > > >>>>>>>>>> Hi Suren, > > >>>>>>>>>> > > >>>>>>>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): > > >>>>>>>>>> > > >>>>>>>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > > >>>>>>>>>> @uffd-common.c:648) > > >>>>>>>>>> > > >>>>>>>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > > >>>>>>>>>> happy to go deeper if you can direct. > > >>>>>>>>> > > >>>>>>>>> Does it trigger reliably? Which pagesize is that kernel using? > > >>>>>>>> > > >>>>>>>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > > >>>>>>>> for full config. > > >>>>>>>> > > >>>>>>>>> > > >>>>>>>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > > >>>>>>>>> default_huge_page_size(), which reads the default hugetlb size. > > >>>>>>>> > > >>>>>>>> My kernel command line is explicitly seting the default huge page size to 2M. > > >>>>>>>> > > >>>>>>> > > >>>>>>> Okay, so that likely won't affect it. > > >>>>>>> > > >>>>>>> I can only guess that it has to do with the alignment of the virtual > > >>>>>>> area we are testing with, and that we do seem to get more odd patterns > > >>>>>>> on arm64. > > >>>>>>> > > >>>>>>> uffd_move_test_common() is a bit more elaborate, but if we aligned the > > >>>>>>> src+start area up, surely "step_count" cannot be left unmodified? > > >>>>>>> > > >>>>>>> So assuming we get either an unaligned source or an unaligned dst from > > >>>>>>> mmap(), I am not convinced that we won't be moving areas that are not > > >>>>>>> necessarily fully backed by PMDs and maybe don't even fall into the VMA > > >>>>>>> of interest? > > >>>>>>> > > >>>>>>> Not sure if that could trigger the THP splitting issue, though. > > >>>>>>> > > >>>>>>> But I just quickly scanned that test setup, could be I am missing > > >>>>>>> something. It might make sense to just print the mmap'ed range and the > > >>>>>>> actual ranges we are trying to move. Maybe something "obvious" can be > > >>>>>>> observed. > > >>>>>> > > >>>>>> I was able to reproduce the issue on an Android device and after > > >>>>>> implementing David's suggestions to split the large folio and after > > >>>>>> replacing default_huge_page_size() with read_pmd_pagesize(), the > > >>>>>> move-pmd test started working for me. Ryan, could you please apply > > >>>>>> attached patches (over mm-unstable) and try the test again? > > >>>>> > > >>>>> Yep, all fixed with those patches! > > >>>> > > >>>> Great! Thanks for testing and confirming. I'll post an updated > > >>>> patchset later today and will ask Andrew to replace the current one > > >>>> with it. > > >>>> I'll also look into the reasons we need to split PMD on ARM64 in this > > >>>> test. It's good that this happened and we were able to test the PMD > > >>>> split path but I'm curious about the reason. It's possible my address > > >>>> alignment calculations are somehow incorrect. > > >>> > > >>> I only skimmed the diff briefly, but likely you also want to try > > >>> splitting in move_pages_pte(), if you encounter an already-pte-mapped THP. > > >> > > >> Huh, good point. I might be able to move the folio splitting code into > > >> pte-mapped case and do a retry after splitting. That should minimize > > >> the additional code required. Will do and post a new set shortly. > > >> Thanks! > > > > > > Was planning to post an update today but need some more time. Will try > > > to send it tomorrow. > > > > It would be great to have tests that cover these cases (having to > > PTE-map a PMD-mapped THP, and stumbling over an already-PTE-mapped one). > > Agree. Let me post the new version so that mm-unstable does not > produce these failures and will start working on covering additional > cases in the tests. The new patchset is almost ready, just finishing > final tests. Posted v6 at https://lore.kernel.org/all/20231206103702.3873743-1-surenb@google.com/. Changes are listed in the cover letter. Andrew, could you please replace the current v5 version in mm-unstable with this new one? Thanks, Suren. > > > > > -- > > Cheers, > > > > David / dhildenb > >
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); } +int move_page(int ufd, unsigned long offset, unsigned long len) +{ + struct uffdio_move uffdio_move; + + if (offset + len > nr_pages * page_size) + err("unexpected offset %lu and length %lu\n", offset, len); + uffdio_move.dst = (unsigned long) area_dst + offset; + uffdio_move.src = (unsigned long) area_src + offset; + uffdio_move.len = len; + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; + uffdio_move.move = 0; + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { + /* real retval in uffdio_move.move */ + if (uffdio_move.move != -EEXIST) + err("UFFDIO_MOVE error: %"PRId64, + (int64_t)uffdio_move.move); + wake_range(ufd, uffdio_move.dst, len); + } else if (uffdio_move.move != len) { + err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move); + } else + return 1; + return 0; +} + int uffd_open_dev(unsigned int flags) { int fd, uffd; diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index 774595ee629e..cb055282c89c 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -119,6 +119,7 @@ void wp_range(int ufd, __u64 start, __u64 len, bool wp); void uffd_handle_page_fault(struct uffd_msg *msg, struct uffd_args *args); int __copy_page(int ufd, unsigned long offset, bool retry, bool wp); int copy_page(int ufd, unsigned long offset, bool wp); +int move_page(int ufd, unsigned long offset, unsigned long len); void *uffd_poll_thread(void *arg); int uffd_open_dev(unsigned int flags); diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index debc423bdbf4..e4e271511db9 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -23,6 +23,9 @@ #define MEM_ALL (MEM_ANON | MEM_SHMEM | MEM_SHMEM_PRIVATE | \ MEM_HUGETLB | MEM_HUGETLB_PRIVATE) +#define ALIGN_UP(x, align_to) \ + ((__typeof__(x))((((unsigned long)(x)) + ((align_to)-1)) & ~((align_to)-1))) + struct mem_type { const char *name; unsigned int mem_flag; @@ -1064,6 +1067,178 @@ static void uffd_poison_test(uffd_test_args_t *targs) uffd_test_pass(); } +static void +uffd_move_handle_fault_common(struct uffd_msg *msg, struct uffd_args *args, + unsigned long len) +{ + unsigned long offset; + + if (msg->event != UFFD_EVENT_PAGEFAULT) + err("unexpected msg event %u", msg->event); + + if (msg->arg.pagefault.flags & + (UFFD_PAGEFAULT_FLAG_WP | UFFD_PAGEFAULT_FLAG_MINOR | UFFD_PAGEFAULT_FLAG_WRITE)) + err("unexpected fault type %llu", msg->arg.pagefault.flags); + + offset = (char *)(unsigned long)msg->arg.pagefault.address - area_dst; + offset &= ~(len-1); + + if (move_page(uffd, offset, len)) + args->missing_faults++; +} + +static void uffd_move_handle_fault(struct uffd_msg *msg, + struct uffd_args *args) +{ + uffd_move_handle_fault_common(msg, args, page_size); +} + +static void uffd_move_pmd_handle_fault(struct uffd_msg *msg, + struct uffd_args *args) +{ + uffd_move_handle_fault_common(msg, args, default_huge_page_size()); +} + +static void +uffd_move_test_common(uffd_test_args_t *targs, unsigned long chunk_size, + void (*handle_fault)(struct uffd_msg *msg, struct uffd_args *args)) +{ + unsigned long nr; + pthread_t uffd_mon; + char c; + unsigned long long count; + struct uffd_args args = { 0 }; + char *orig_area_src, *orig_area_dst; + unsigned long step_size, step_count; + unsigned long src_offs = 0; + unsigned long dst_offs = 0; + + /* Prevent source pages from being mapped more than once */ + if (madvise(area_src, nr_pages * page_size, MADV_DONTFORK)) + err("madvise(MADV_DONTFORK) failure"); + + if (uffd_register(uffd, area_dst, nr_pages * page_size, + true, false, false)) + err("register failure"); + + args.handle_fault = handle_fault; + if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args)) + err("uffd_poll_thread create"); + + step_size = chunk_size / page_size; + step_count = nr_pages / step_size; + + if (step_size > page_size) { + char *aligned_src = ALIGN_UP(area_src, chunk_size); + char *aligned_dst = ALIGN_UP(area_dst, chunk_size); + + if (aligned_src != area_src || aligned_dst != area_dst) { + src_offs = (aligned_src - area_src) / page_size; + dst_offs = (aligned_dst - area_dst) / page_size; + step_count--; + } + orig_area_src = area_src; + orig_area_dst = area_dst; + area_src = aligned_src; + area_dst = aligned_dst; + } + + /* + * Read each of the pages back using the UFFD-registered mapping. We + * expect that the first time we touch a page, it will result in a missing + * fault. uffd_poll_thread will resolve the fault by moving source + * page to destination. + */ + for (nr = 0; nr < step_count * step_size; nr += step_size) { + unsigned long i; + + /* Check area_src content */ + for (i = 0; i < step_size; i++) { + count = *area_count(area_src, nr + i); + if (count != count_verify[src_offs + nr + i]) + err("nr %lu source memory invalid %llu %llu\n", + nr + i, count, count_verify[src_offs + nr + i]); + } + + /* Faulting into area_dst should move the page or the huge page */ + for (i = 0; i < step_size; i++) { + count = *area_count(area_dst, nr + i); + if (count != count_verify[dst_offs + nr + i]) + err("nr %lu memory corruption %llu %llu\n", + nr, count, count_verify[dst_offs + nr + i]); + } + + /* Re-check area_src content which should be empty */ + for (i = 0; i < step_size; i++) { + count = *area_count(area_src, nr + i); + if (count != 0) + err("nr %lu move failed %llu %llu\n", + nr, count, count_verify[src_offs + nr + i]); + } + } + if (step_size > page_size) { + area_src = orig_area_src; + area_dst = orig_area_dst; + } + + if (write(pipefd[1], &c, sizeof(c)) != sizeof(c)) + err("pipe write"); + if (pthread_join(uffd_mon, NULL)) + err("join() failed"); + + if (args.missing_faults != step_count || args.minor_faults != 0) + uffd_test_fail("stats check error"); + else + uffd_test_pass(); +} + +static void uffd_move_test(uffd_test_args_t *targs) +{ + uffd_move_test_common(targs, page_size, uffd_move_handle_fault); +} + +static void uffd_move_pmd_test(uffd_test_args_t *targs) +{ + uffd_move_test_common(targs, default_huge_page_size(), + uffd_move_pmd_handle_fault); +} + +static int prevent_hugepages(const char **errmsg) +{ + /* This should be done before source area is populated */ + if (madvise(area_src, nr_pages * page_size, MADV_NOHUGEPAGE)) { + /* Ignore only if CONFIG_TRANSPARENT_HUGEPAGE=n */ + if (errno != EINVAL) { + if (errmsg) + *errmsg = "madvise(MADV_NOHUGEPAGE) failed"; + return -errno; + } + } + return 0; +} + +static int request_hugepages(const char **errmsg) +{ + /* This should be done before source area is populated */ + if (madvise(area_src, nr_pages * page_size, MADV_HUGEPAGE)) { + if (errmsg) { + *errmsg = (errno == EINVAL) ? + "CONFIG_TRANSPARENT_HUGEPAGE is not set" : + "madvise(MADV_HUGEPAGE) failed"; + } + return -errno; + } + return 0; +} + +struct uffd_test_case_ops uffd_move_test_case_ops = { + .post_alloc = prevent_hugepages, +}; + +struct uffd_test_case_ops uffd_move_test_pmd_case_ops = { + .post_alloc = request_hugepages, +}; + /* * Test the returned uffdio_register.ioctls with different register modes. * Note that _UFFDIO_ZEROPAGE is tested separately in the zeropage test. @@ -1141,6 +1316,20 @@ uffd_test_case_t uffd_tests[] = { .mem_targets = MEM_ALL, .uffd_feature_required = 0, }, + { + .name = "move", + .uffd_fn = uffd_move_test, + .mem_targets = MEM_ANON, + .uffd_feature_required = UFFD_FEATURE_MOVE, + .test_case_ops = &uffd_move_test_case_ops, + }, + { + .name = "move-pmd", + .uffd_fn = uffd_move_pmd_test, + .mem_targets = MEM_ANON, + .uffd_feature_required = UFFD_FEATURE_MOVE, + .test_case_ops = &uffd_move_test_pmd_case_ops, + }, { .name = "wp-fork", .uffd_fn = uffd_wp_fork_test,
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)