| Message ID | 20220913052203.177071-1-apopple@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hmm-tests: Fix migrate_dirty_page test | expand |
On 13.9.2022 8.22, Alistair Popple wrote: > As noted by John Hubbard the original test relied on side effects of the > implementation of migrate_vma_setup() to detect if pages had been > swapped to disk or not. This is subject to change in future so > explicitly check for swap entries via pagemap instead. Fix a spelling > mistake while we're at it. > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > Fixes: 5cc88e844e87 ("selftests/hmm-tests: add test for dirty bits") > --- > tools/testing/selftests/vm/hmm-tests.c | 50 +++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c > index 70fdb49b59ed..b5f6a7dc1f12 100644 > --- a/tools/testing/selftests/vm/hmm-tests.c > +++ b/tools/testing/selftests/vm/hmm-tests.c > @@ -1261,9 +1261,47 @@ static int destroy_cgroup(void) > return 0; > } > > +static uint64_t get_pfn(int fd, uint64_t ptr) > +{ > + uint64_t pfn; > + int ret; > + > + ret = pread(fd, &pfn, sizeof(ptr), > + (uint64_t) ptr / getpagesize() * sizeof(ptr)); > + if (ret != sizeof(ptr)) > + return 0; > + > + return pfn; > +} > + > +#define PAGEMAP_SWAPPED (1ULL << 62) > + > +/* Returns true if at least one page in the range is on swap */ > +static bool pages_swapped(void *ptr, unsigned long pages) > +{ > + uint64_t pfn; > + int fd = open("/proc/self/pagemap", O_RDONLY); > + unsigned long i; > + > + if (fd < 0) > + return false; > + > + for (i = 0; i < pages; i++) { > + pfn = get_pfn(fd, (uint64_t) ptr + i * getpagesize()); > + > + if (pfn & PAGEMAP_SWAPPED) { > + close(fd); > + return true; > + } > + } > + > + close(fd); > + return false; > +} > + > /* > * Try and migrate a dirty page that has previously been swapped to disk. This > - * checks that we don't loose dirty bits. > + * checks that we don't lose dirty bits. > */ > TEST_F(hmm, migrate_dirty_page) > { > @@ -1300,6 +1338,10 @@ TEST_F(hmm, migrate_dirty_page) > > ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30)); > > + /* Make sure at least some pages got paged to disk. */ > + if (!pages_swapped(buffer->ptr, npages)) > + SKIP(return, "Pages weren't swapped when they should have been"); > + > /* Fault pages back in from swap as clean pages */ > for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i) > tmp += ptr[i]; > @@ -1309,10 +1351,10 @@ TEST_F(hmm, migrate_dirty_page) > ptr[i] = i; > > /* > - * Attempt to migrate memory to device, which should fail because > - * hopefully some pages are backed by swap storage. > + * Attempt to migrate memory to device. This might fail if some pages > + * are/were backed by swap but that's ok. > */ > - ASSERT_TRUE(hmm_migrate_sys_to_dev(self->fd, buffer, npages)); > + hmm_migrate_sys_to_dev(self->fd, buffer, npages); > > ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30)); > Reviewed-by: Mika Penttilä <mpenttil@redhat.com>
On 13.09.22 07:22, Alistair Popple wrote: > As noted by John Hubbard the original test relied on side effects of the > implementation of migrate_vma_setup() to detect if pages had been > swapped to disk or not. This is subject to change in future so > explicitly check for swap entries via pagemap instead. Fix a spelling > mistake while we're at it. > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > Fixes: 5cc88e844e87 ("selftests/hmm-tests: add test for dirty bits") > --- > tools/testing/selftests/vm/hmm-tests.c | 50 +++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c > index 70fdb49b59ed..b5f6a7dc1f12 100644 > --- a/tools/testing/selftests/vm/hmm-tests.c > +++ b/tools/testing/selftests/vm/hmm-tests.c > @@ -1261,9 +1261,47 @@ static int destroy_cgroup(void) > return 0; > } > > +static uint64_t get_pfn(int fd, uint64_t ptr) > +{ > + uint64_t pfn; > + int ret; > + > + ret = pread(fd, &pfn, sizeof(ptr), > + (uint64_t) ptr / getpagesize() * sizeof(ptr)); > + if (ret != sizeof(ptr)) > + return 0; > + > + return pfn; > +} > + > +#define PAGEMAP_SWAPPED (1ULL << 62) > + > +/* Returns true if at least one page in the range is on swap */ > +static bool pages_swapped(void *ptr, unsigned long pages) > +{ > + uint64_t pfn; > + int fd = open("/proc/self/pagemap", O_RDONLY); > + unsigned long i; > + > + if (fd < 0) > + return false; > + > + for (i = 0; i < pages; i++) { > + pfn = get_pfn(fd, (uint64_t) ptr + i * getpagesize()); > + > + if (pfn & PAGEMAP_SWAPPED) { > + close(fd); > + return true; > + } We do have pagemap_get_entry() in vm_util.c to query the pagemap entry. Can you further, add pagemap_is_swapped() to vm_util.c? I'll be also needing that (including a variant for testing a range) in anon COW tests.
David Hildenbrand <david@redhat.com> writes: > On 13.09.22 07:22, Alistair Popple wrote: >> As noted by John Hubbard the original test relied on side effects of the >> implementation of migrate_vma_setup() to detect if pages had been >> swapped to disk or not. This is subject to change in future so >> explicitly check for swap entries via pagemap instead. Fix a spelling >> mistake while we're at it. >> Signed-off-by: Alistair Popple <apopple@nvidia.com> >> Fixes: 5cc88e844e87 ("selftests/hmm-tests: add test for dirty bits") >> --- >> tools/testing/selftests/vm/hmm-tests.c | 50 +++++++++++++++++++++++--- >> 1 file changed, 46 insertions(+), 4 deletions(-) >> diff --git a/tools/testing/selftests/vm/hmm-tests.c >> b/tools/testing/selftests/vm/hmm-tests.c >> index 70fdb49b59ed..b5f6a7dc1f12 100644 >> --- a/tools/testing/selftests/vm/hmm-tests.c >> +++ b/tools/testing/selftests/vm/hmm-tests.c >> @@ -1261,9 +1261,47 @@ static int destroy_cgroup(void) >> return 0; >> } >> +static uint64_t get_pfn(int fd, uint64_t ptr) >> +{ >> + uint64_t pfn; >> + int ret; >> + >> + ret = pread(fd, &pfn, sizeof(ptr), >> + (uint64_t) ptr / getpagesize() * sizeof(ptr)); >> + if (ret != sizeof(ptr)) >> + return 0; >> + >> + return pfn; >> +} >> + >> +#define PAGEMAP_SWAPPED (1ULL << 62) >> + >> +/* Returns true if at least one page in the range is on swap */ >> +static bool pages_swapped(void *ptr, unsigned long pages) >> +{ >> + uint64_t pfn; >> + int fd = open("/proc/self/pagemap", O_RDONLY); >> + unsigned long i; >> + >> + if (fd < 0) >> + return false; >> + >> + for (i = 0; i < pages; i++) { >> + pfn = get_pfn(fd, (uint64_t) ptr + i * getpagesize()); >> + >> + if (pfn & PAGEMAP_SWAPPED) { >> + close(fd); >> + return true; >> + } > > We do have pagemap_get_entry() in vm_util.c to query the pagemap entry. Thanks. I'd missed that, although `grep pagemap tools/testing/selftests/vm` suggests I'm not the first to follow a tradition of open-coding this :-) But there's no need to perpetuate that tradition, so will redo this to use vm_util.c instead. > Can you further, add pagemap_is_swapped() to vm_util.c? > > I'll be also needing that (including a variant for testing a range) in anon COW > tests.
On 13.09.22 10:20, Alistair Popple wrote: > > David Hildenbrand <david@redhat.com> writes: > >> On 13.09.22 07:22, Alistair Popple wrote: >>> As noted by John Hubbard the original test relied on side effects of the >>> implementation of migrate_vma_setup() to detect if pages had been >>> swapped to disk or not. This is subject to change in future so >>> explicitly check for swap entries via pagemap instead. Fix a spelling >>> mistake while we're at it. >>> Signed-off-by: Alistair Popple <apopple@nvidia.com> >>> Fixes: 5cc88e844e87 ("selftests/hmm-tests: add test for dirty bits") >>> --- >>> tools/testing/selftests/vm/hmm-tests.c | 50 +++++++++++++++++++++++--- >>> 1 file changed, 46 insertions(+), 4 deletions(-) >>> diff --git a/tools/testing/selftests/vm/hmm-tests.c >>> b/tools/testing/selftests/vm/hmm-tests.c >>> index 70fdb49b59ed..b5f6a7dc1f12 100644 >>> --- a/tools/testing/selftests/vm/hmm-tests.c >>> +++ b/tools/testing/selftests/vm/hmm-tests.c >>> @@ -1261,9 +1261,47 @@ static int destroy_cgroup(void) >>> return 0; >>> } >>> +static uint64_t get_pfn(int fd, uint64_t ptr) >>> +{ >>> + uint64_t pfn; >>> + int ret; >>> + >>> + ret = pread(fd, &pfn, sizeof(ptr), >>> + (uint64_t) ptr / getpagesize() * sizeof(ptr)); >>> + if (ret != sizeof(ptr)) >>> + return 0; >>> + >>> + return pfn; >>> +} >>> + >>> +#define PAGEMAP_SWAPPED (1ULL << 62) >>> + >>> +/* Returns true if at least one page in the range is on swap */ >>> +static bool pages_swapped(void *ptr, unsigned long pages) >>> +{ >>> + uint64_t pfn; >>> + int fd = open("/proc/self/pagemap", O_RDONLY); >>> + unsigned long i; >>> + >>> + if (fd < 0) >>> + return false; >>> + >>> + for (i = 0; i < pages; i++) { >>> + pfn = get_pfn(fd, (uint64_t) ptr + i * getpagesize()); >>> + >>> + if (pfn & PAGEMAP_SWAPPED) { >>> + close(fd); >>> + return true; >>> + } >> >> We do have pagemap_get_entry() in vm_util.c to query the pagemap entry. > > Thanks. I'd missed that, although `grep pagemap > tools/testing/selftests/vm` suggests I'm not the first to follow a > tradition of open-coding this :-) > > But there's no need to perpetuate that tradition, so will redo this to > use vm_util.c instead. Yeah, we just recently factored stuff out into there. I'll be factoring out more in my upcoming tests from the madv_populate tests.
diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c index 70fdb49b59ed..b5f6a7dc1f12 100644 --- a/tools/testing/selftests/vm/hmm-tests.c +++ b/tools/testing/selftests/vm/hmm-tests.c @@ -1261,9 +1261,47 @@ static int destroy_cgroup(void) return 0; } +static uint64_t get_pfn(int fd, uint64_t ptr) +{ + uint64_t pfn; + int ret; + + ret = pread(fd, &pfn, sizeof(ptr), + (uint64_t) ptr / getpagesize() * sizeof(ptr)); + if (ret != sizeof(ptr)) + return 0; + + return pfn; +} + +#define PAGEMAP_SWAPPED (1ULL << 62) + +/* Returns true if at least one page in the range is on swap */ +static bool pages_swapped(void *ptr, unsigned long pages) +{ + uint64_t pfn; + int fd = open("/proc/self/pagemap", O_RDONLY); + unsigned long i; + + if (fd < 0) + return false; + + for (i = 0; i < pages; i++) { + pfn = get_pfn(fd, (uint64_t) ptr + i * getpagesize()); + + if (pfn & PAGEMAP_SWAPPED) { + close(fd); + return true; + } + } + + close(fd); + return false; +} + /* * Try and migrate a dirty page that has previously been swapped to disk. This - * checks that we don't loose dirty bits. + * checks that we don't lose dirty bits. */ TEST_F(hmm, migrate_dirty_page) { @@ -1300,6 +1338,10 @@ TEST_F(hmm, migrate_dirty_page) ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30)); + /* Make sure at least some pages got paged to disk. */ + if (!pages_swapped(buffer->ptr, npages)) + SKIP(return, "Pages weren't swapped when they should have been"); + /* Fault pages back in from swap as clean pages */ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i) tmp += ptr[i]; @@ -1309,10 +1351,10 @@ TEST_F(hmm, migrate_dirty_page) ptr[i] = i; /* - * Attempt to migrate memory to device, which should fail because - * hopefully some pages are backed by swap storage. + * Attempt to migrate memory to device. This might fail if some pages + * are/were backed by swap but that's ok. */ - ASSERT_TRUE(hmm_migrate_sys_to_dev(self->fd, buffer, npages)); + hmm_migrate_sys_to_dev(self->fd, buffer, npages); ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30));
As noted by John Hubbard the original test relied on side effects of the implementation of migrate_vma_setup() to detect if pages had been swapped to disk or not. This is subject to change in future so explicitly check for swap entries via pagemap instead. Fix a spelling mistake while we're at it. Signed-off-by: Alistair Popple <apopple@nvidia.com> Fixes: 5cc88e844e87 ("selftests/hmm-tests: add test for dirty bits") --- tools/testing/selftests/vm/hmm-tests.c | 50 +++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 4 deletions(-)