It turned out that an even dozen patches were required in order to get the selftests building cleanly, and all running, once again. I made it worse on myself by insisting on using clang, which seems to uncover a few more warnings than gcc these days.
So I still haven't gotten to my original goal of running a new HMM test that Alistair handed me (it's not here yet), but at least this fixes everything I ran into just now.
John Hubbard (12): selftests/mm: fix uffd-stress unused function warning selftests/mm: fix unused variable warning in hugetlb-madvise.c selftests/mm: fix unused variable warning in migration.c selftests/mm: fix a char* assignment in mlock2-tests.c selftests/mm: fix invocation of tests that are run via shell scripts selftests/mm: .gitignore: add mkdirty, va_high_addr_switch selftests/mm: set -Wno-format-security to avoid uffd build warnings selftests/mm: fix a "possibly uninitialized" warning in pkey-x86.h selftests/mm: move psize(), pshift() into vm_utils.c selftests/mm: move uffd* routines from vm_util.c to uffd-common.c selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE
tools/testing/selftests/mm/.gitignore | 2 + tools/testing/selftests/mm/Makefile | 9 +- tools/testing/selftests/mm/cow.c | 7 -- tools/testing/selftests/mm/hugepage-mremap.c | 2 +- tools/testing/selftests/mm/hugetlb-madvise.c | 2 +- tools/testing/selftests/mm/khugepaged.c | 10 -- .../selftests/mm/ksm_functional_tests.c | 2 +- tools/testing/selftests/mm/migration.c | 2 +- tools/testing/selftests/mm/mlock2-tests.c | 2 +- tools/testing/selftests/mm/pkey-x86.h | 2 +- tools/testing/selftests/mm/run_vmtests.sh | 6 +- tools/testing/selftests/mm/uffd-common.c | 105 +++++++++++++++++ tools/testing/selftests/mm/uffd-common.h | 29 ++++- tools/testing/selftests/mm/uffd-stress.c | 10 -- tools/testing/selftests/mm/vm_util.c | 106 ++---------------- tools/testing/selftests/mm/vm_util.h | 36 ++---- 16 files changed, 170 insertions(+), 162 deletions(-)
base-commit: 929ed21dfdb6ee94391db51c9eedb63314ef6847
uffd_minor_feature() was unused. Remove it in order to fix the associated clang build warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/uffd-stress.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c index f1ad9eef1c3a..995ff13e74c7 100644 --- a/tools/testing/selftests/mm/uffd-stress.c +++ b/tools/testing/selftests/mm/uffd-stress.c @@ -88,16 +88,6 @@ static void uffd_stats_reset(struct uffd_args *args, unsigned long n_cpus) } }
-static inline uint64_t uffd_minor_feature(void) -{ - if (test_type == TEST_HUGETLB && map_shared) - return UFFD_FEATURE_MINOR_HUGETLBFS; - else if (test_type == TEST_SHMEM) - return UFFD_FEATURE_MINOR_SHMEM; - else - return 0; -} - static void *locking_thread(void *arg) { unsigned long cpu = (unsigned long) arg;
On 02.06.23 03:33, John Hubbard wrote:
uffd_minor_feature() was unused. Remove it in order to fix the associated clang build warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/uffd-stress.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c index f1ad9eef1c3a..995ff13e74c7 100644 --- a/tools/testing/selftests/mm/uffd-stress.c +++ b/tools/testing/selftests/mm/uffd-stress.c @@ -88,16 +88,6 @@ static void uffd_stats_reset(struct uffd_args *args, unsigned long n_cpus) } } -static inline uint64_t uffd_minor_feature(void) -{
- if (test_type == TEST_HUGETLB && map_shared)
return UFFD_FEATURE_MINOR_HUGETLBFS;- else if (test_type == TEST_SHMEM)
return UFFD_FEATURE_MINOR_SHMEM;- else
return 0;-}
- static void *locking_thread(void *arg) { unsigned long cpu = (unsigned long) arg;
Reviewed-by: David Hildenbrand david@redhat.com
On Thu, Jun 01, 2023 at 06:33:47PM -0700, John Hubbard wrote:
uffd_minor_feature() was unused. Remove it in order to fix the associated clang build warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com
Reviewed-by: Peter Xu peterx@redhat.com
The dummy variable is required in order to make this work, so declare it as volatile in order to avoid the clang compiler warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/hugetlb-madvise.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/hugetlb-madvise.c b/tools/testing/selftests/mm/hugetlb-madvise.c index 28426e30d9bc..3296ccaf7525 100644 --- a/tools/testing/selftests/mm/hugetlb-madvise.c +++ b/tools/testing/selftests/mm/hugetlb-madvise.c @@ -65,7 +65,7 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
void read_fault_pages(void *addr, unsigned long nr_pages) { - unsigned long dummy = 0; + volatile unsigned long dummy = 0; unsigned long i;
for (i = 0; i < nr_pages; i++)
On 02.06.23 03:33, John Hubbard wrote:
The dummy variable is required in order to make this work, so declare it as volatile in order to avoid the clang compiler warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/hugetlb-madvise.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/hugetlb-madvise.c b/tools/testing/selftests/mm/hugetlb-madvise.c index 28426e30d9bc..3296ccaf7525 100644 --- a/tools/testing/selftests/mm/hugetlb-madvise.c +++ b/tools/testing/selftests/mm/hugetlb-madvise.c @@ -65,7 +65,7 @@ void write_fault_pages(void *addr, unsigned long nr_pages) void read_fault_pages(void *addr, unsigned long nr_pages) {
- unsigned long dummy = 0;
- volatile unsigned long dummy = 0; unsigned long i;
for (i = 0; i < nr_pages; i++)
The compiler can still decide to optimize it all out, because it's not a global variable.
Placing a
asm volatile("" : "+r" (dummy));
after the write tells the compiler that the value will be read and cannot be optimized out (we use that trick in the cow selftest and I've been using it in QEMU for the same purpose as well).
On 6/2/23 03:01, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
The dummy variable is required in order to make this work, so declare it as volatile in order to avoid the clang compiler warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/hugetlb-madvise.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/hugetlb-madvise.c b/tools/testing/selftests/mm/hugetlb-madvise.c index 28426e30d9bc..3296ccaf7525 100644 --- a/tools/testing/selftests/mm/hugetlb-madvise.c +++ b/tools/testing/selftests/mm/hugetlb-madvise.c @@ -65,7 +65,7 @@ void write_fault_pages(void *addr, unsigned long nr_pages) void read_fault_pages(void *addr, unsigned long nr_pages) { - unsigned long dummy = 0; + volatile unsigned long dummy = 0; unsigned long i; for (i = 0; i < nr_pages; i++)
The compiler can still decide to optimize it all out, because it's not a global variable.
Placing a
asm volatile("" : "+r" (dummy));
after the write tells the compiler that the value will be read and cannot be optimized out (we use that trick in the cow selftest and I've been using it in QEMU for the same purpose as well).
Done. I will send a v2 with this, thanks!
thanks,
Declare the variable as volatile, in order to avoid the clang compiler warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c index 1cec8425e3ca..2dede662ff43 100644 --- a/tools/testing/selftests/mm/migration.c +++ b/tools/testing/selftests/mm/migration.c @@ -95,7 +95,7 @@ int migrate(uint64_t *ptr, int n1, int n2)
void *access_mem(void *ptr) { - uint64_t y = 0; + volatile uint64_t y = 0; volatile uint64_t *x = ptr;
while (1) {
On 02.06.23 03:33, John Hubbard wrote:
Declare the variable as volatile, in order to avoid the clang compiler warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c index 1cec8425e3ca..2dede662ff43 100644 --- a/tools/testing/selftests/mm/migration.c +++ b/tools/testing/selftests/mm/migration.c @@ -95,7 +95,7 @@ int migrate(uint64_t *ptr, int n1, int n2) void *access_mem(void *ptr) {
- uint64_t y = 0;
- volatile uint64_t y = 0; volatile uint64_t *x = ptr;
while (1) {
Same comment as for previous patch.
On 6/2/23 03:02, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
Declare the variable as volatile, in order to avoid the clang compiler warning.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c index 1cec8425e3ca..2dede662ff43 100644 --- a/tools/testing/selftests/mm/migration.c +++ b/tools/testing/selftests/mm/migration.c @@ -95,7 +95,7 @@ int migrate(uint64_t *ptr, int n1, int n2) void *access_mem(void *ptr) { - uint64_t y = 0; + volatile uint64_t y = 0; volatile uint64_t *x = ptr; while (1) {
Same comment as for previous patch.
Yes. In fact I'll merge this one into the previous patch, since they are so similar and small, and have the same fix approach.
thanks,
The stop variable is a char*, so use "\0" when assigning to it, rather than attempting to assign a character type. This was generating a warning when compiling with clang.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/mlock2-tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c index 11b2301f3aa3..8ee95077dc25 100644 --- a/tools/testing/selftests/mm/mlock2-tests.c +++ b/tools/testing/selftests/mm/mlock2-tests.c @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area) printf("cannot parse /proc/self/maps\n"); goto out; } - stop = '\0'; + stop = "\0";
sscanf(line, "%lx", &start); sscanf(end_addr, "%lx", &end);
On 02.06.23 03:33, John Hubbard wrote:
The stop variable is a char*, so use "\0" when assigning to it, rather than attempting to assign a character type. This was generating a warning when compiling with clang.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/mlock2-tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c index 11b2301f3aa3..8ee95077dc25 100644 --- a/tools/testing/selftests/mm/mlock2-tests.c +++ b/tools/testing/selftests/mm/mlock2-tests.c @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area) printf("cannot parse /proc/self/maps\n"); goto out; }
stop = '\0';
stop = "\0";sscanf(line, "%lx", &start); sscanf(end_addr, "%lx", &end);
I'm probably missing something, but what is the stop variable supposed to do here? It's completely unused, no?
if (!strchr(end_addr, ' ')) { printf("cannot parse /proc/self/maps\n"); goto out; }
On Fri, Jun 02, 2023 at 12:04:57PM +0200, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
The stop variable is a char*, so use "\0" when assigning to it, rather than attempting to assign a character type. This was generating a warning when compiling with clang.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/mlock2-tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c index 11b2301f3aa3..8ee95077dc25 100644 --- a/tools/testing/selftests/mm/mlock2-tests.c +++ b/tools/testing/selftests/mm/mlock2-tests.c @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area) printf("cannot parse /proc/self/maps\n"); goto out; }
stop = '\0';
stop = "\0";I'm probably missing something, but what is the stop variable supposed to do here? It's completely unused, no?
if (!strchr(end_addr, ' ')) { printf("cannot parse /proc/self/maps\n"); goto out; }
I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot since the sscanf() just worked..
On 6/2/23 08:24, Peter Xu wrote:
On Fri, Jun 02, 2023 at 12:04:57PM +0200, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
The stop variable is a char*, so use "\0" when assigning to it, rather than attempting to assign a character type. This was generating a warning when compiling with clang.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/mlock2-tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c index 11b2301f3aa3..8ee95077dc25 100644 --- a/tools/testing/selftests/mm/mlock2-tests.c +++ b/tools/testing/selftests/mm/mlock2-tests.c @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area) printf("cannot parse /proc/self/maps\n"); goto out; }
stop = '\0';
stop = "\0"; sscanf(line, "%lx", &start); sscanf(end_addr, "%lx", &end);I'm probably missing something, but what is the stop variable supposed to do here? It's completely unused, no?
if (!strchr(end_addr, ' ')) { printf("cannot parse /proc/self/maps\n"); goto out; }
Yes it is! I certainly had tunnel vision on that one. I've changed the patch to simply delete that line, for v2, thanks.
I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot since the sscanf() just worked..
Maybe, yes. Hard to tell the original intent at this point...it might have been used in an early draft version of the loop that didn't get posted, perhaps. thanks,
On Fri, Jun 02, 2023 at 11:52:42AM -0700, John Hubbard wrote:
On 6/2/23 08:24, Peter Xu wrote:
On Fri, Jun 02, 2023 at 12:04:57PM +0200, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
The stop variable is a char*, so use "\0" when assigning to it, rather than attempting to assign a character type. This was generating a warning when compiling with clang.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/mlock2-tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c index 11b2301f3aa3..8ee95077dc25 100644 --- a/tools/testing/selftests/mm/mlock2-tests.c +++ b/tools/testing/selftests/mm/mlock2-tests.c @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area) printf("cannot parse /proc/self/maps\n"); goto out; }
stop = '\0';
stop = "\0"; sscanf(line, "%lx", &start); sscanf(end_addr, "%lx", &end);I'm probably missing something, but what is the stop variable supposed to do here? It's completely unused, no?
if (!strchr(end_addr, ' ')) { printf("cannot parse /proc/self/maps\n"); goto out; }
Yes it is! I certainly had tunnel vision on that one. I've changed the patch to simply delete that line, for v2, thanks.
I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot since the sscanf() just worked..
Maybe, yes. Hard to tell the original intent at this point...it might have been used in an early draft version of the loop that didn't get posted, perhaps.
I'm pretty sure of it.. see the pattern:
end_addr = strchr(line, '-'); if (!end_addr) { printf("cannot parse /proc/self/maps\n"); goto out; } *end_addr = '\0';
And...
stop = strchr(end_addr, ' '); if (!stop) { printf("cannot parse /proc/self/maps\n"); goto out; } stop = '\0'; <------------------- only diff here
On 6/5/23 08:38, Peter Xu wrote: ...
I'm probably missing something, but what is the stop variable supposed to do here? It's completely unused, no?
if (!strchr(end_addr, ' ')) { printf("cannot parse /proc/self/maps\n"); goto out; }
Yes it is! I certainly had tunnel vision on that one. I've changed the patch to simply delete that line, for v2, thanks.
I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot since the sscanf() just worked..
Maybe, yes. Hard to tell the original intent at this point...it might have been used in an early draft version of the loop that didn't get posted, perhaps.
I'm pretty sure of it.. see the pattern:
end_addr = strchr(line, '-'); if (!end_addr) { printf("cannot parse /proc/self/maps\n"); goto out; } *end_addr = '\0';And...
stop = strchr(end_addr, ' '); if (!stop) { printf("cannot parse /proc/self/maps\n"); goto out; } stop = '\0'; <------------------- only diff here
Yes, and that pattern shows why it wants to be "*stop = '\0';", but it doesn't show why the author wasted a line of code in the first place, setting a variable that is not used afterwards.
In other words, changing this to "*stop = '\0';" would make it look pretty, but it's a non-functional line of code to add. Which is why I think it should just be deleted at this point.
thanks,
We cannot depend upon git to reliably retain the executable bit on shell scripts, or so I was told several years ago while working on this same run_vmtests.sh script. And sure enough, things such as test_hmm.sh are lately failing to run, due to lacking execute permissions.
A nice clean way to fix this would have been to use TEST_PROGS instead of TEST_FILES for the .sh scripts here. That tells the selftest framework to run these (and emit a warning if the files are not executable, but still run them anyway).
Unfortunately, run_vmtests.sh has its own run_test() routine, which does *not* do the right thing for shell scripts.
Fix this by explicitly adding "bash" to each of the shell script invocations. Leave fixing the overall approach to another day.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/run_vmtests.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index 4893eb60d96d..8f81432e4bac 100644 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -242,18 +242,18 @@ if [ $VADDR64 -ne 0 ]; then if [ "$ARCH" == "$ARCH_ARM64" ]; then echo 6 > /proc/sys/vm/nr_hugepages fi - CATEGORY="hugevm" run_test ./va_high_addr_switch.sh + CATEGORY="hugevm" run_test bash ./va_high_addr_switch.sh if [ "$ARCH" == "$ARCH_ARM64" ]; then echo $prev_nr_hugepages > /proc/sys/vm/nr_hugepages fi fi # VADDR64
# vmalloc stability smoke test -CATEGORY="vmalloc" run_test ./test_vmalloc.sh smoke +CATEGORY="vmalloc" run_test bash ./test_vmalloc.sh smoke
CATEGORY="mremap" run_test ./mremap_dontunmap
-CATEGORY="hmm" run_test ./test_hmm.sh smoke +CATEGORY="hmm" run_test bash ./test_hmm.sh smoke
# MADV_POPULATE_READ and MADV_POPULATE_WRITE tests CATEGORY="madv_populate" run_test ./madv_populate
On 02.06.23 03:33, John Hubbard wrote:
We cannot depend upon git to reliably retain the executable bit on shell scripts, or so I was told several years ago while working on this same run_vmtests.sh script. And sure enough, things such as test_hmm.sh are lately failing to run, due to lacking execute permissions.
A nice clean way to fix this would have been to use TEST_PROGS instead of TEST_FILES for the .sh scripts here. That tells the selftest framework to run these (and emit a warning if the files are not executable, but still run them anyway).
Unfortunately, run_vmtests.sh has its own run_test() routine, which does *not* do the right thing for shell scripts.
Fix this by explicitly adding "bash" to each of the shell script invocations. Leave fixing the overall approach to another day.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/run_vmtests.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index 4893eb60d96d..8f81432e4bac 100644 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -242,18 +242,18 @@ if [ $VADDR64 -ne 0 ]; then if [ "$ARCH" == "$ARCH_ARM64" ]; then echo 6 > /proc/sys/vm/nr_hugepages fi
- CATEGORY="hugevm" run_test ./va_high_addr_switch.sh
- CATEGORY="hugevm" run_test bash ./va_high_addr_switch.sh if [ "$ARCH" == "$ARCH_ARM64" ]; then echo $prev_nr_hugepages > /proc/sys/vm/nr_hugepages fi fi # VADDR64
# vmalloc stability smoke test -CATEGORY="vmalloc" run_test ./test_vmalloc.sh smoke +CATEGORY="vmalloc" run_test bash ./test_vmalloc.sh smoke CATEGORY="mremap" run_test ./mremap_dontunmap -CATEGORY="hmm" run_test ./test_hmm.sh smoke +CATEGORY="hmm" run_test bash ./test_hmm.sh smoke # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests CATEGORY="madv_populate" run_test ./madv_populate
Sounds hacky, but if it gets the job done
Acked-by: David Hildenbrand david@redhat.com
On 6/2/23 03:05, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
We cannot depend upon git to reliably retain the executable bit on shell scripts, or so I was told several years ago while working on this same run_vmtests.sh script. And sure enough, things such as test_hmm.sh are lately failing to run, due to lacking execute permissions.
A nice clean way to fix this would have been to use TEST_PROGS instead of TEST_FILES for the .sh scripts here. That tells the selftest framework to run these (and emit a warning if the files are not executable, but still run them anyway).
Actually, for the record (and I'll update this in v2), the above is inaccurate, because run_vmtests.sh aspires to be the only TEST_PROGS item here. And I see that the framework does already work if-and-only-if invoked via Make, as in "make run_tests".
However,
a) Many people naturally expect to run test scripts without (unnecessarily!) involving Make, and
b) Based on some experience in building and using various test frameworks over many years, I'd claim that it's better to use shell scripts to collect and manage tests and test scripts, rather than involving Make. Make is a limited, specialized language and is better at handling builds and dependencies.
So the "make run_tests" is a convenience, but it should not be the only way to launch a test run. So we still want to fix this up.
Unfortunately, run_vmtests.sh has its own run_test() routine, which does *not* do the right thing for shell scripts.
Fix this by explicitly adding "bash" to each of the shell script invocations. Leave fixing the overall approach to another day.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/run_vmtests.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index 4893eb60d96d..8f81432e4bac 100644 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -242,18 +242,18 @@ if [ $VADDR64 -ne 0 ]; then if [ "$ARCH" == "$ARCH_ARM64" ]; then echo 6 > /proc/sys/vm/nr_hugepages fi - CATEGORY="hugevm" run_test ./va_high_addr_switch.sh + CATEGORY="hugevm" run_test bash ./va_high_addr_switch.sh if [ "$ARCH" == "$ARCH_ARM64" ]; then echo $prev_nr_hugepages > /proc/sys/vm/nr_hugepages fi fi # VADDR64 # vmalloc stability smoke test -CATEGORY="vmalloc" run_test ./test_vmalloc.sh smoke +CATEGORY="vmalloc" run_test bash ./test_vmalloc.sh smoke CATEGORY="mremap" run_test ./mremap_dontunmap -CATEGORY="hmm" run_test ./test_hmm.sh smoke +CATEGORY="hmm" run_test bash ./test_hmm.sh smoke # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests CATEGORY="madv_populate" run_test ./madv_populate
Sounds hacky, but if it gets the job done
Yes. It's also hacky that we can't just invoke shell scripts like normal programs. This limitation hurts my sense of "things should be more perfect!". :)
Acked-by: David Hildenbrand david@redhat.com
Thanks for the ack.
On Thu, Jun 01, 2023 at 06:33:51PM -0700, John Hubbard wrote:
We cannot depend upon git to reliably retain the executable bit on shell scripts, or so I was told several years ago while working on this same run_vmtests.sh script. And sure enough, things such as test_hmm.sh are lately failing to run, due to lacking execute permissions.
A nice clean way to fix this would have been to use TEST_PROGS instead of TEST_FILES for the .sh scripts here. That tells the selftest framework to run these (and emit a warning if the files are not executable, but still run them anyway).
Unfortunately, run_vmtests.sh has its own run_test() routine, which does *not* do the right thing for shell scripts.
Fix this by explicitly adding "bash" to each of the shell script invocations. Leave fixing the overall approach to another day.
Is it possible someone just doesn't have "bash" at all? I used to only use "sh" without bash installed I think, but that was not on Linux, so I'm not sure how much that applies..
Maybe use $(SHELL)? I saw a bunch of usage in the tree too.
On 6/2/23 08:34, Peter Xu wrote:
On Thu, Jun 01, 2023 at 06:33:51PM -0700, John Hubbard wrote:
We cannot depend upon git to reliably retain the executable bit on shell scripts, or so I was told several years ago while working on this same run_vmtests.sh script. And sure enough, things such as test_hmm.sh are lately failing to run, due to lacking execute permissions.
A nice clean way to fix this would have been to use TEST_PROGS instead of TEST_FILES for the .sh scripts here. That tells the selftest framework to run these (and emit a warning if the files are not executable, but still run them anyway).
Unfortunately, run_vmtests.sh has its own run_test() routine, which does *not* do the right thing for shell scripts.
Fix this by explicitly adding "bash" to each of the shell script invocations. Leave fixing the overall approach to another day.
Is it possible someone just doesn't have "bash" at all? I used to only use
Well, maybe [1]. But that someone won't be running these tests as-is, because the tests explicitly require bash, even before this patch.
"sh" without bash installed I think, but that was not on Linux, so I'm not sure how much that applies..
sh invocations are for when you want to express that this script should avoid using bash-specific things, in order to ensure portability to other environments.
But here, the run_vmtests.sh file requires bash already, as per the first line:
#!/bin/bash
...which is ultimately why I decided to use bash, rather than sh here.
Maybe use $(SHELL)? I saw a bunch of usage in the tree too.
That's more of a Makefile construct that you are seeing, and only in a few odd Makefiles. Recall that in Make, $(SHELL) has the same effect that ${SHELL} has in bash/sh, by the way: dereferencing a variable.
And Make's "$(shell ...)" command is what is normally used to *run* a shell command, in the kernel's build system.
Having said all that, I will take a quick look at what it would take to shift over to the selftest framework's run_test() instead, in order to avoid this ugly "fix".
[1] https://www.bleepingcomputer.com/news/linux/kali-linux-20204-switches-the-de...
thanks,
On Fri, Jun 02, 2023 at 12:19:17PM -0700, John Hubbard wrote:
On 6/2/23 08:34, Peter Xu wrote:
On Thu, Jun 01, 2023 at 06:33:51PM -0700, John Hubbard wrote:
We cannot depend upon git to reliably retain the executable bit on shell scripts, or so I was told several years ago while working on this same run_vmtests.sh script. And sure enough, things such as test_hmm.sh are lately failing to run, due to lacking execute permissions.
A nice clean way to fix this would have been to use TEST_PROGS instead of TEST_FILES for the .sh scripts here. That tells the selftest framework to run these (and emit a warning if the files are not executable, but still run them anyway).
Unfortunately, run_vmtests.sh has its own run_test() routine, which does *not* do the right thing for shell scripts.
Fix this by explicitly adding "bash" to each of the shell script invocations. Leave fixing the overall approach to another day.
Is it possible someone just doesn't have "bash" at all? I used to only use
Well, maybe [1]. But that someone won't be running these tests as-is, because the tests explicitly require bash, even before this patch.
"sh" without bash installed I think, but that was not on Linux, so I'm not sure how much that applies..
sh invocations are for when you want to express that this script should avoid using bash-specific things, in order to ensure portability to other environments.
But here, the run_vmtests.sh file requires bash already, as per the first line:
#!/bin/bash...which is ultimately why I decided to use bash, rather than sh here.
That one can be easily override with $XXX run_vmtests.sh, hard-coded "bash" in Makefiles can't, afaiu.
Maybe use $(SHELL)? I saw a bunch of usage in the tree too.
That's more of a Makefile construct that you are seeing, and only in a few odd Makefiles. Recall that in Make, $(SHELL) has the same effect that ${SHELL} has in bash/sh, by the way: dereferencing a variable.
And Make's "$(shell ...)" command is what is normally used to *run* a shell command, in the kernel's build system.
Having said all that, I will take a quick look at what it would take to shift over to the selftest framework's run_test() instead, in order to avoid this ugly "fix".
Just to mention that I was not talking about $(shell ...), but the environment var $(SHELL), or "env | grep SHELL".
Please feel free to have a look at tools/perf/arch/x86/Makefile.
Thanks,
On 6/2/23 14:36, Peter Xu wrote: ...
But here, the run_vmtests.sh file requires bash already, as per the first line:
#!/bin/bash...which is ultimately why I decided to use bash, rather than sh here.
That one can be easily override with $XXX run_vmtests.sh, hard-coded "bash" in Makefiles can't, afaiu.
Yes, but then you'd have to deal with the rest of the kernel, and bash is just completely woven into the whole thing. Just in the selftests alone, there are dozens or hundreds of direct invocations.
$ git grep -w bash | wc -l 1146
$ cd tools/testing/selftests/
$ git grep -w bash | wc -l 560
$ git grep -w bash | grep -v '/bin' | wc -l 113
That ship really has sailed: it's not practical to expect that kind of portability here.
...
Just to mention that I was not talking about $(shell ...), but the environment var $(SHELL), or "env | grep SHELL".
Please feel free to have a look at tools/perf/arch/x86/Makefile.
Yes, but that is a *Makefile*. (And only one out of 145, the others do not use this.) There is no use of SHELL outside of Makefiles, nor in fact anywhere in the kernel.
thanks,
These new build products were left out of .gitignore, so add them now.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/.gitignore | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore index 8917455f4f51..ab215303d8e9 100644 --- a/tools/testing/selftests/mm/.gitignore +++ b/tools/testing/selftests/mm/.gitignore @@ -39,3 +39,5 @@ local_config.h local_config.mk ksm_functional_tests mdwe_test +mkdirty +va_high_addr_switch \ No newline at end of file
On 02.06.23 03:33, John Hubbard wrote:
These new build products were left out of .gitignore, so add them now.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/.gitignore | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore index 8917455f4f51..ab215303d8e9 100644 --- a/tools/testing/selftests/mm/.gitignore +++ b/tools/testing/selftests/mm/.gitignore @@ -39,3 +39,5 @@ local_config.h local_config.mk ksm_functional_tests mdwe_test +mkdirty +va_high_addr_switch \ No newline at end of file
Reviewed-by: David Hildenbrand david@redhat.com
The uffd_test_start() is perhaps a little too elaborate about how it dispatches tests, leading to a clang warning that looks roughly like this:
"uffd-unit-tests.c:1198:20: warning: format string is not a string literal (potentially insecure) [-Wformat-security] ...note: treat the string as an argument to avoid this. uffd_test_start(test_name); "
However, it doesn't seem worth it to rewrite the way uffd_test_start() works, given that these tests are already deeply unsafe to begin with.
Fix this by just disabling the compiler warning, but only for uffd-unit-tests.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/Makefile | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 23af4633f0f4..473bf1811552 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -170,6 +170,8 @@ $(OUTPUT)/ksm_tests: LDLIBS += -lnuma
$(OUTPUT)/migration: LDLIBS += -lnuma
+$(OUTPUT)/uffd-unit-tests: CFLAGS += -Wno-format-security + local_config.mk local_config.h: check_config.sh /bin/sh ./check_config.sh $(CC)
On 02.06.23 03:33, John Hubbard wrote:
The uffd_test_start() is perhaps a little too elaborate about how it dispatches tests, leading to a clang warning that looks roughly like this:
"uffd-unit-tests.c:1198:20: warning: format string is not a string literal (potentially insecure) [-Wformat-security] ...note: treat the string as an argument to avoid this. uffd_test_start(test_name); "
However, it doesn't seem worth it to rewrite the way uffd_test_start() works, given that these tests are already deeply unsafe to begin with.
Fix this by just disabling the compiler warning, but only for uffd-unit-tests.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/Makefile | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 23af4633f0f4..473bf1811552 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -170,6 +170,8 @@ $(OUTPUT)/ksm_tests: LDLIBS += -lnuma $(OUTPUT)/migration: LDLIBS += -lnuma +$(OUTPUT)/uffd-unit-tests: CFLAGS += -Wno-format-security
- local_config.mk local_config.h: check_config.sh /bin/sh ./check_config.sh $(CC)
Maybe the following will silence the warning by removing test_name completely:
--- tools/testing/selftests/mm/uffd-unit-tests.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 269c86768a02..15c76ce972be 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -1149,7 +1149,6 @@ int main(int argc, char *argv[]) uffd_test_case_t *test; mem_type_t *mem_type; uffd_test_args_t args; - char test_name[128]; const char *errmsg; int has_uffd, opt; int i, j; @@ -1192,10 +1191,8 @@ int main(int argc, char *argv[]) mem_type = &mem_types[j]; if (!(test->mem_targets & mem_type->mem_flag)) continue; - snprintf(test_name, sizeof(test_name), - "%s on %s", test->name, mem_type->name);
- uffd_test_start(test_name); + uffd_test_start("%s on %s", test->name, mem_type->name); if (!uffd_feature_supported(test)) { uffd_test_skip("feature missing"); continue;
Still gives me
Testing register-ioctls on anon... done Testing register-ioctls on shmem... done Testing register-ioctls on shmem-private... done Testing register-ioctls on hugetlb... skipped [reason: memory allocation failed] Testing register-ioctls on hugetlb-private... skipped [reason: memory allocation failed] ...
On 6/2/23 03:15, David Hildenbrand wrote: ...
Maybe the following will silence the warning by removing test_name completely:
tools/testing/selftests/mm/uffd-unit-tests.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 269c86768a02..15c76ce972be 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -1149,7 +1149,6 @@ int main(int argc, char *argv[]) uffd_test_case_t *test; mem_type_t *mem_type; uffd_test_args_t args; - char test_name[128]; const char *errmsg; int has_uffd, opt; int i, j; @@ -1192,10 +1191,8 @@ int main(int argc, char *argv[]) mem_type = &mem_types[j]; if (!(test->mem_targets & mem_type->mem_flag)) continue; - snprintf(test_name, sizeof(test_name), - "%s on %s", test->name, mem_type->name);
- uffd_test_start(test_name); + uffd_test_start("%s on %s", test->name, mem_type->name); if (!uffd_feature_supported(test)) { uffd_test_skip("feature missing"); continue;
Yes, that does clean up one of the two warnings nicely. I'll do that instead -Wno-format-security, thanks.
That still leaves another similar warning that fires for "errmsg". However, that one is easily fixed because the associated macro turns out to be unnecessary, because all callers pass a simple char* string, without any format specifiers.
So just turning one macro into a C function fixes that, leaving us with this (plus your fix from above):
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 269c86768a02..04d91f144d1c 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -109,12 +109,11 @@ static void uffd_test_pass(void) ksft_inc_fail_cnt(); \ } while (0)
-#define uffd_test_skip(...) do { \ - printf("skipped [reason: "); \ - printf(__VA_ARGS__); \ - printf("]\n"); \ - ksft_inc_xskip_cnt(); \ - } while (0) +static void uffd_test_skip(const char *message) +{ + printf("skipped [reason: %s]\n", message); + ksft_inc_xskip_cnt(); +}
I'll send that in a v2.
thanks,
This fixes a real bug, too, because xstate_size() was assuming that the stack variable xstate_size was initialized to zero. That's not guaranteed nor even especially likely.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/pkey-x86.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/pkey-x86.h b/tools/testing/selftests/mm/pkey-x86.h index 72c14cd3ddc7..e32ae8a1cd99 100644 --- a/tools/testing/selftests/mm/pkey-x86.h +++ b/tools/testing/selftests/mm/pkey-x86.h @@ -132,7 +132,7 @@ int pkey_reg_xstate_offset(void) unsigned int ecx; unsigned int edx; int xstate_offset; - int xstate_size; + int xstate_size = 0; unsigned long XSTATE_CPUID = 0xd; int leaf;
On 02.06.23 03:33, John Hubbard wrote:
This fixes a real bug, too, because xstate_size() was assuming that the stack variable xstate_size was initialized to zero. That's not guaranteed nor even especially likely.
Signed-off-by: John Hubbard jhubbard@nvidia.com
Reviewed-by: David Hildenbrand david@redhat.com
This is in preparation for linking test programs with both vm_utils.c and uffd-common.c. The static inline routines would prevent that, and there is no particular need for inlining here, so turn these into normal functions that are more flexible to build and link.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/vm_util.c | 14 ++++++++++++++ tools/testing/selftests/mm/vm_util.h | 16 ++-------------- 2 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c index 9b06a5034808..01296c17df02 100644 --- a/tools/testing/selftests/mm/vm_util.c +++ b/tools/testing/selftests/mm/vm_util.c @@ -301,3 +301,17 @@ int uffd_get_features(uint64_t *features)
return 0; } + +unsigned int psize(void) +{ + if (!__page_size) + __page_size = sysconf(_SC_PAGESIZE); + return __page_size; +} + +unsigned int pshift(void) +{ + if (!__page_shift) + __page_shift = (ffsl(psize()) - 1); + return __page_shift; +} diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index b950bd16083a..232ffeb5805c 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -3,7 +3,6 @@ #include <stdbool.h> #include <sys/mman.h> #include <err.h> -#include <string.h> /* ffsl() */ #include <unistd.h> /* _SC_PAGESIZE */
#define BIT_ULL(nr) (1ULL << (nr)) @@ -17,19 +16,8 @@ extern unsigned int __page_size; extern unsigned int __page_shift;
-static inline unsigned int psize(void) -{ - if (!__page_size) - __page_size = sysconf(_SC_PAGESIZE); - return __page_size; -} - -static inline unsigned int pshift(void) -{ - if (!__page_shift) - __page_shift = (ffsl(psize()) - 1); - return __page_shift; -} +unsigned int psize(void); +unsigned int pshift(void);
uint64_t pagemap_get_entry(int fd, char *start); bool pagemap_is_softdirty(int fd, char *start);
On 02.06.23 03:33, John Hubbard wrote:
This is in preparation for linking test programs with both vm_utils.c and uffd-common.c. The static inline routines would prevent that, and there is no particular need for inlining here, so turn these into normal functions that are more flexible to build and link.
I'm probably missing something important, but isn't it the most common thing to use "static inline" across multiple objects that we then link?
Hope you can enlighten me what the real issue here is.
On 6/2/23 03:19, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
This is in preparation for linking test programs with both vm_utils.c and uffd-common.c. The static inline routines would prevent that, and there is no particular need for inlining here, so turn these into normal functions that are more flexible to build and link.
I'm probably missing something important, but isn't it the most common thing to use "static inline" across multiple objects that we then link?
Yes, absolutely. I've just had my confidence in things shaken by some of the quirks in the selftests framework, but you're right.
Hope you can enlighten me what the real issue here is.
At this point it looks like a header file inclusion mess. As usual, including header files from other header files leads to problems, and in this case I'm now seeing vm_utils.h included from uffd-common.h, causing multiple definitions of these static inline functions.
If I try to undo that it generates a boatload of new errors, so I'm thinking to just change the commit message to explain that I'm moving this to a normal function in order to avoid the above situation.
thanks,
This is where they belong, and this makes it cleaner to apply a follow-up fix to the uffd builds.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/Makefile | 7 +- tools/testing/selftests/mm/hugepage-mremap.c | 2 +- .../selftests/mm/ksm_functional_tests.c | 2 +- tools/testing/selftests/mm/uffd-common.c | 105 ++++++++++++++++++ tools/testing/selftests/mm/uffd-common.h | 12 +- tools/testing/selftests/mm/vm_util.c | 104 ----------------- tools/testing/selftests/mm/vm_util.h | 10 -- 7 files changed, 122 insertions(+), 120 deletions(-)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 473bf1811552..9bf3305b7dea 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -109,8 +109,11 @@ include ../lib.mk
$(TEST_GEN_PROGS): vm_util.c
-$(OUTPUT)/uffd-stress: uffd-common.c -$(OUTPUT)/uffd-unit-tests: uffd-common.c +$(OUTPUT)/uffd-stress: uffd-common.c +$(OUTPUT)/uffd-unit-tests: uffd-common.c +$(OUTPUT)/hugepage-mremap: uffd-common.c +$(OUTPUT)/write_to_hugetlbfs: uffd-common.c +$(OUTPUT)/ksm_functional_tests: uffd-common.c
ifeq ($(MACHINE),x86_64) BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32)) diff --git a/tools/testing/selftests/mm/hugepage-mremap.c b/tools/testing/selftests/mm/hugepage-mremap.c index cabd0084f57b..8158fe909f5e 100644 --- a/tools/testing/selftests/mm/hugepage-mremap.c +++ b/tools/testing/selftests/mm/hugepage-mremap.c @@ -24,7 +24,7 @@ #include <sys/ioctl.h> #include <string.h> #include <stdbool.h> -#include "vm_util.h" +#include "uffd-common.h"
#define DEFAULT_LENGTH_MB 10UL #define MB_TO_BYTES(x) (x * 1024 * 1024) diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c index 26853badae70..648188ad73fa 100644 --- a/tools/testing/selftests/mm/ksm_functional_tests.c +++ b/tools/testing/selftests/mm/ksm_functional_tests.c @@ -22,7 +22,7 @@ #include <linux/userfaultfd.h>
#include "../kselftest.h" -#include "vm_util.h" +#include "uffd-common.h"
#define KiB 1024u #define MiB (1024 * KiB) diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index 61c6250adf93..e1ad63668a05 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -6,6 +6,7 @@ */
#include "uffd-common.h" +#include "vm_util.h"
#define BASE_PMD_ADDR ((void *)(1UL << 30))
@@ -616,3 +617,107 @@ int copy_page(int ufd, unsigned long offset, bool wp) { return __copy_page(ufd, offset, false, wp); } + +/* If `ioctls' non-NULL, the allowed ioctls will be returned into the var */ +int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len, + bool miss, bool wp, bool minor, uint64_t *ioctls) +{ + struct uffdio_register uffdio_register = { 0 }; + uint64_t mode = 0; + int ret = 0; + + if (miss) + mode |= UFFDIO_REGISTER_MODE_MISSING; + if (wp) + mode |= UFFDIO_REGISTER_MODE_WP; + if (minor) + mode |= UFFDIO_REGISTER_MODE_MINOR; + + uffdio_register.range.start = (unsigned long)addr; + uffdio_register.range.len = len; + uffdio_register.mode = mode; + + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) + ret = -errno; + else if (ioctls) + *ioctls = uffdio_register.ioctls; + + return ret; +} + +int uffd_register(int uffd, void *addr, uint64_t len, + bool miss, bool wp, bool minor) +{ + return uffd_register_with_ioctls(uffd, addr, len, + miss, wp, minor, NULL); +} + +int uffd_unregister(int uffd, void *addr, uint64_t len) +{ + struct uffdio_range range = { .start = (uintptr_t)addr, .len = len }; + int ret = 0; + + if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1) + ret = -errno; + + return ret; +} + +int uffd_open_dev(unsigned int flags) +{ + int fd, uffd; + + fd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC); + if (fd < 0) + return fd; + uffd = ioctl(fd, USERFAULTFD_IOC_NEW, flags); + close(fd); + + return uffd; +} + +int uffd_open_sys(unsigned int flags) +{ +#ifdef __NR_userfaultfd + return syscall(__NR_userfaultfd, flags); +#else + return -1; +#endif +} + +int uffd_open(unsigned int flags) +{ + int uffd = uffd_open_sys(flags); + + if (uffd < 0) + uffd = uffd_open_dev(flags); + + return uffd; +} + +int uffd_get_features(uint64_t *features) +{ + struct uffdio_api uffdio_api = { .api = UFFD_API, .features = 0 }; + /* + * This should by default work in most kernels; the feature list + * will be the same no matter what we pass in here. + */ + int fd = uffd_open(UFFD_USER_MODE_ONLY); + + if (fd < 0) + /* Maybe the kernel is older than user-only mode? */ + fd = uffd_open(0); + + if (fd < 0) + return fd; + + if (ioctl(fd, UFFDIO_API, &uffdio_api)) { + close(fd); + return -errno; + } + + *features = uffdio_api.features; + close(fd); + + return 0; +} diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index 6068f2346b86..a1cdb78c0762 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -19,8 +19,6 @@ #include <signal.h> #include <poll.h> #include <string.h> -#include <linux/mman.h> -#include <sys/mman.h> #include <sys/syscall.h> #include <sys/ioctl.h> #include <sys/wait.h> @@ -110,6 +108,16 @@ int __copy_page(int ufd, unsigned long offset, bool retry, bool wp); int copy_page(int ufd, unsigned long offset, bool wp); void *uffd_poll_thread(void *arg);
+int uffd_register(int uffd, void *addr, uint64_t len, + bool miss, bool wp, bool minor); +int uffd_unregister(int uffd, void *addr, uint64_t len); +int uffd_open_dev(unsigned int flags); +int uffd_open_sys(unsigned int flags); +int uffd_open(unsigned int flags); +int uffd_get_features(uint64_t *features); +int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len, + bool miss, bool wp, bool minor, uint64_t *ioctls); + #define TEST_ANON 1 #define TEST_HUGETLB 2 #define TEST_SHMEM 3 diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c index 01296c17df02..c64a0134f83c 100644 --- a/tools/testing/selftests/mm/vm_util.c +++ b/tools/testing/selftests/mm/vm_util.c @@ -198,110 +198,6 @@ unsigned long default_huge_page_size(void) return hps; }
-/* If `ioctls' non-NULL, the allowed ioctls will be returned into the var */ -int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len, - bool miss, bool wp, bool minor, uint64_t *ioctls) -{ - struct uffdio_register uffdio_register = { 0 }; - uint64_t mode = 0; - int ret = 0; - - if (miss) - mode |= UFFDIO_REGISTER_MODE_MISSING; - if (wp) - mode |= UFFDIO_REGISTER_MODE_WP; - if (minor) - mode |= UFFDIO_REGISTER_MODE_MINOR; - - uffdio_register.range.start = (unsigned long)addr; - uffdio_register.range.len = len; - uffdio_register.mode = mode; - - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) - ret = -errno; - else if (ioctls) - *ioctls = uffdio_register.ioctls; - - return ret; -} - -int uffd_register(int uffd, void *addr, uint64_t len, - bool miss, bool wp, bool minor) -{ - return uffd_register_with_ioctls(uffd, addr, len, - miss, wp, minor, NULL); -} - -int uffd_unregister(int uffd, void *addr, uint64_t len) -{ - struct uffdio_range range = { .start = (uintptr_t)addr, .len = len }; - int ret = 0; - - if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1) - ret = -errno; - - return ret; -} - -int uffd_open_dev(unsigned int flags) -{ - int fd, uffd; - - fd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC); - if (fd < 0) - return fd; - uffd = ioctl(fd, USERFAULTFD_IOC_NEW, flags); - close(fd); - - return uffd; -} - -int uffd_open_sys(unsigned int flags) -{ -#ifdef __NR_userfaultfd - return syscall(__NR_userfaultfd, flags); -#else - return -1; -#endif -} - -int uffd_open(unsigned int flags) -{ - int uffd = uffd_open_sys(flags); - - if (uffd < 0) - uffd = uffd_open_dev(flags); - - return uffd; -} - -int uffd_get_features(uint64_t *features) -{ - struct uffdio_api uffdio_api = { .api = UFFD_API, .features = 0 }; - /* - * This should by default work in most kernels; the feature list - * will be the same no matter what we pass in here. - */ - int fd = uffd_open(UFFD_USER_MODE_ONLY); - - if (fd < 0) - /* Maybe the kernel is older than user-only mode? */ - fd = uffd_open(0); - - if (fd < 0) - return fd; - - if (ioctl(fd, UFFDIO_API, &uffdio_api)) { - close(fd); - return -errno; - } - - *features = uffdio_api.features; - close(fd); - - return 0; -} - unsigned int psize(void) { if (!__page_size) diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index 232ffeb5805c..7f5aac0ac680 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -33,16 +33,6 @@ bool check_huge_shmem(void *addr, int nr_hpages, uint64_t hpage_size); int64_t allocate_transhuge(void *ptr, int pagemap_fd); unsigned long default_huge_page_size(void);
-int uffd_register(int uffd, void *addr, uint64_t len, - bool miss, bool wp, bool minor); -int uffd_unregister(int uffd, void *addr, uint64_t len); -int uffd_open_dev(unsigned int flags); -int uffd_open_sys(unsigned int flags); -int uffd_open(unsigned int flags); -int uffd_get_features(uint64_t *features); -int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len, - bool miss, bool wp, bool minor, uint64_t *ioctls); - /* * On ppc64 this will only work with radix 2M hugepage size */
On Thu, Jun 01, 2023 at 06:33:56PM -0700, John Hubbard wrote:
This is where they belong, and this makes it cleaner to apply a follow-up fix to the uffd builds.
Signed-off-by: John Hubbard jhubbard@nvidia.com
Thanks for further looking into this.
I'm fine to move it over if you think proper, but just to mention I had those in vm_utils.h just because I left all uffd specific tests shared code in uffd-common.h, so my plan was uffd-common.h shouldn't be included in most test cases except uffd tests.
I'm not sure whether we can just make your next patch of "ifndef.." into vm_utils.h to avoid the movement, or is it a must?
Thanks,
On 6/2/23 08:59, Peter Xu wrote:
On Thu, Jun 01, 2023 at 06:33:56PM -0700, John Hubbard wrote:
This is where they belong, and this makes it cleaner to apply a follow-up fix to the uffd builds.
Signed-off-by: John Hubbard jhubbard@nvidia.com
Thanks for further looking into this.
I'm fine to move it over if you think proper, but just to mention I had those in vm_utils.h just because I left all uffd specific tests shared code in uffd-common.h, so my plan was uffd-common.h shouldn't be included in most test cases except uffd tests.
I think we're in agreement that we want to only include uffd-common.h where it's actually required. Likewise with the uffd*() routines. So I would like to still move this over, yes, just to have things in their best-named location.
I'm not sure whether we can just make your next patch of "ifndef.." into vm_utils.h to avoid the movement, or is it a must?
Actually, I think I can drop the next patch entirely, based on Muhammad's observation that we should be doing a "make headers" to pull in those items. I'll have more to say over on that thread.
thanks,
On Fri, Jun 02, 2023 at 03:11:52PM -0700, John Hubbard wrote:
On 6/2/23 08:59, Peter Xu wrote:
On Thu, Jun 01, 2023 at 06:33:56PM -0700, John Hubbard wrote:
This is where they belong, and this makes it cleaner to apply a follow-up fix to the uffd builds.
Signed-off-by: John Hubbard jhubbard@nvidia.com
Thanks for further looking into this.
I'm fine to move it over if you think proper, but just to mention I had those in vm_utils.h just because I left all uffd specific tests shared code in uffd-common.h, so my plan was uffd-common.h shouldn't be included in most test cases except uffd tests.
I think we're in agreement that we want to only include uffd-common.h where it's actually required. Likewise with the uffd*() routines. So I would like to still move this over, yes, just to have things in their best-named location.
Sorry I didn't get it - e.g. I'm confused why we need to export uffd_test_ops into ksm unit test, it doesn't make much sense to me..
If you think vm_util.h is a name too common to contain uffd helpers, shall we create another vm_util_uffd.h just to put the uffd helpers?
Just see what's there in uffd-common.h, which is still ugly (I could look into it some other day):
extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap; extern int uffd, uffd_flags, finished, *pipefd, test_type; extern bool map_shared; extern bool test_uffdio_wp; extern unsigned long long *count_verify; extern volatile bool test_uffdio_copy_eexist;
extern uffd_test_ops_t anon_uffd_test_ops; extern uffd_test_ops_t shmem_uffd_test_ops; extern uffd_test_ops_t hugetlb_uffd_test_ops; extern uffd_test_ops_t *uffd_test_ops;
and more.
That's why I think this header should not better be included by anyone else besides uffd-stress.c and uffd-unit-tests.c for now.
I'm not sure whether we can just make your next patch of "ifndef.." into vm_utils.h to avoid the movement, or is it a must?
Actually, I think I can drop the next patch entirely, based on Muhammad's observation that we should be doing a "make headers" to pull in those items. I'll have more to say over on that thread.
Sure, great if the local headers will work. Thanks.
On 6/2/23 15:38, Peter Xu wrote: ...
I'm fine to move it over if you think proper, but just to mention I had those in vm_utils.h just because I left all uffd specific tests shared code in uffd-common.h, so my plan was uffd-common.h shouldn't be included in most test cases except uffd tests.
I think we're in agreement that we want to only include uffd-common.h where it's actually required. Likewise with the uffd*() routines. So I would like to still move this over, yes, just to have things in their best-named location.
Sorry I didn't get it - e.g. I'm confused why we need to export uffd_test_ops into ksm unit test, it doesn't make much sense to me..
Oh, I see what you mean, finally. Yes. ksm should not need that.
If you think vm_util.h is a name too common to contain uffd helpers, shall
Right, given the presence of uffd-common.[chg], I really want to avoid putting the uffd helpers somewhere else...
we create another vm_util_uffd.h just to put the uffd helpers?
Just see what's there in uffd-common.h, which is still ugly (I could look into it some other day):
Good point.
extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap; extern int uffd, uffd_flags, finished, *pipefd, test_type; extern bool map_shared; extern bool test_uffdio_wp; extern unsigned long long *count_verify; extern volatile bool test_uffdio_copy_eexist;
extern uffd_test_ops_t anon_uffd_test_ops; extern uffd_test_ops_t shmem_uffd_test_ops; extern uffd_test_ops_t hugetlb_uffd_test_ops; extern uffd_test_ops_t *uffd_test_ops;
and more.
That's why I think this header should not better be included by anyone else besides uffd-stress.c and uffd-unit-tests.c for now.
OK, I think I can arrange things to meet that requirement. Let me take another shot at it.
thanks,
On 6/2/23 15:52, John Hubbard wrote:
On 6/2/23 15:38, Peter Xu wrote: ...
I think we're in agreement that we want to only include uffd-common.h where it's actually required. Likewise with the uffd*() routines. So I would like to still move this over, yes, just to have things in their best-named location.
Sorry I didn't get it - e.g. I'm confused why we need to export uffd_test_ops into ksm unit test, it doesn't make much sense to me..
Oh, I see what you mean, finally. Yes. ksm should not need that.
...whoops, correction, our very own David Hildenbrand recently made changes that contradict the claim that "ksm and uffd selftests are independent". In fact, ksm now *intentionally* depends upon uffd, as of commit 93fb70aa5904c ("selftests/vm: add KSM unmerge tests"), aha!
That added commit added a call to test_unmerge_uffd_wp(), to ksm_functional_tests.c .
So this needs to stay approximately as-is, it seems.
thanks,
On Fri, Jun 02, 2023 at 05:43:19PM -0700, John Hubbard wrote:
On 6/2/23 15:52, John Hubbard wrote:
On 6/2/23 15:38, Peter Xu wrote: ...
I think we're in agreement that we want to only include uffd-common.h where it's actually required. Likewise with the uffd*() routines. So I would like to still move this over, yes, just to have things in their best-named location.
Sorry I didn't get it - e.g. I'm confused why we need to export uffd_test_ops into ksm unit test, it doesn't make much sense to me..
Oh, I see what you mean, finally. Yes. ksm should not need that.
...whoops, correction, our very own David Hildenbrand recently made changes that contradict the claim that "ksm and uffd selftests are independent". In fact, ksm now *intentionally* depends upon uffd, as of commit 93fb70aa5904c ("selftests/vm: add KSM unmerge tests"), aha!
That added commit added a call to test_unmerge_uffd_wp(), to ksm_functional_tests.c .
So this needs to stay approximately as-is, it seems.
So I think it depends on what is "as-is" to me in the above sentence. :)
test_unmerge_uffd_wp() impled its own uffd ioctls, and it still doesn't use any of uffd-common.h of now (e.g. uffd_test_ops).
IMHO if we want we can let test_unmerge_uffd_wp() reuse either uffd_get_features(), uffd_open(), uffd_register() etc., but still all of them are provided by vm_util.h not uffd-common.h for now, and that's intended (vm_util.h can contain uffd helpers, or whatever helpers as long as generic mm/ unit tests need).
We can even move wp_range() from uffd-common.[ch] into vm_utils.[ch], then it can also share that (need to replace err(), that's uffd-common specific). Not necessary anything must be done in this series, though.
Thanks,
On 6/2/23 18:18, Peter Xu wrote: ...
...whoops, correction, our very own David Hildenbrand recently made changes that contradict the claim that "ksm and uffd selftests are independent". In fact, ksm now *intentionally* depends upon uffd, as of commit 93fb70aa5904c ("selftests/vm: add KSM unmerge tests"), aha!
That added commit added a call to test_unmerge_uffd_wp(), to ksm_functional_tests.c .
So this needs to stay approximately as-is, it seems.
So I think it depends on what is "as-is" to me in the above sentence. :)
test_unmerge_uffd_wp() impled its own uffd ioctls, and it still doesn't use any of uffd-common.h of now (e.g. uffd_test_ops).
IMHO if we want we can let test_unmerge_uffd_wp() reuse either uffd_get_features(), uffd_open(), uffd_register() etc., but still all of them are provided by vm_util.h not uffd-common.h for now, and that's intended (vm_util.h can contain uffd helpers, or whatever helpers as long as generic mm/ unit tests need).
ksm_functional_tests.c calls uffd_register(). That's about as clear as it gets: this file distinctly depends upon uffd test functionality.
The goal here is to put uffd*() routines into uffd-common.[ch], and everything else into vm_utils.[ch]. Because that's what you do, when you have such named files.
Putting uffd*() routines somewhere other than uffd-common.* requires some...reason. And all I've heard so far is, "it was already scrambled--as intended, don't mess with it!" :)
We can even move wp_range() from uffd-common.[ch] into vm_utils.[ch], then it can also share that (need to replace err(), that's uffd-common specific). Not necessary anything must be done in this series, though.
But wp_range(), despite its generic-sounding name, is another example of something that remains tightly coupled to the uffd code: it uses UFFDIO_WRITEPROTECT to get its work done.
So I'd recommend leaving this one in uffd-common.c.
thanks,
UFFDIO_CONTINUE_MODE_WP, UFFD_FEATURE_WP_UNPOPULATED, USERFAULTFD_IOC, and USERFAULTFD_IOC_NEW are needed lately, but they are not in my host (Arch Linux) distro's userfaultfd.h yet. So put them in here.
A better approach would be to include the uapi version of userfaultfd.h from the kernel tree, but that currently fails with rather difficult linker problems (__packed is defined multiple times, ugg), so defer that to another day and just fix the build for now.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/uffd-common.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index a1cdb78c0762..98847e41ecf9 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -36,6 +36,23 @@
#define UFFD_FLAGS (O_CLOEXEC | O_NONBLOCK | UFFD_USER_MODE_ONLY)
+#ifndef UFFDIO_CONTINUE_MODE_WP +#define UFFDIO_CONTINUE_MODE_WP ((__u64)1<<1) +#endif + +#ifndef UFFD_FEATURE_WP_UNPOPULATED +#define UFFD_FEATURE_WP_UNPOPULATED (1<<13) +#endif + +/* ioctls for /dev/userfaultfd */ +#ifndef USERFAULTFD_IOC +#define USERFAULTFD_IOC 0xAA +#endif + +#ifndef USERFAULTFD_IOC_NEW +#define USERFAULTFD_IOC_NEW _IO(USERFAULTFD_IOC, 0x00) +#endif + #define _err(fmt, ...) \ do { \ int ret = errno; \
On 02.06.23 03:33, John Hubbard wrote:
UFFDIO_CONTINUE_MODE_WP, UFFD_FEATURE_WP_UNPOPULATED, USERFAULTFD_IOC, and USERFAULTFD_IOC_NEW are needed lately, but they are not in my host (Arch Linux) distro's userfaultfd.h yet. So put them in here.
A better approach would be to include the uapi version of userfaultfd.h from the kernel tree, but that currently fails with rather difficult linker problems (__packed is defined multiple times, ugg), so defer that to another day and just fix the build for now.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/uffd-common.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index a1cdb78c0762..98847e41ecf9 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -36,6 +36,23 @@ #define UFFD_FLAGS (O_CLOEXEC | O_NONBLOCK | UFFD_USER_MODE_ONLY) +#ifndef UFFDIO_CONTINUE_MODE_WP +#define UFFDIO_CONTINUE_MODE_WP ((__u64)1<<1) +#endif
+#ifndef UFFD_FEATURE_WP_UNPOPULATED +#define UFFD_FEATURE_WP_UNPOPULATED (1<<13) +#endif
+/* ioctls for /dev/userfaultfd */ +#ifndef USERFAULTFD_IOC +#define USERFAULTFD_IOC 0xAA +#endif
+#ifndef USERFAULTFD_IOC_NEW +#define USERFAULTFD_IOC_NEW _IO(USERFAULTFD_IOC, 0x00) +#endif
- #define _err(fmt, ...) \ do { \ int ret = errno; \
Unfortunately, that seems to be the ugly way to handle this because including the in-tree headers seems to not work and I yet haven't figured out why (there were some changes back and forth so I lost track).
CFLAGS = -Wall -I $(top_srcdir) -I $(top_srcdir)/tools/include/uapi $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
Acked-by: David Hildenbrand david@redhat.com
On 6/2/23 03:23, David Hildenbrand wrote: ...
Unfortunately, that seems to be the ugly way to handle this because including the in-tree headers seems to not work and I yet haven't figured out why (there were some changes back and forth so I lost track).
Yes, ugly, and based on Muhammad's response, I plan on dropping this patch entirely, in fact.
CFLAGS = -Wall -I $(top_srcdir) -I $(top_srcdir)/tools/include/uapi $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
Acked-by: David Hildenbrand david@redhat.com
thanks,
On 03.06.23 00:20, John Hubbard wrote:
On 6/2/23 03:23, David Hildenbrand wrote: ...
Unfortunately, that seems to be the ugly way to handle this because including the in-tree headers seems to not work and I yet haven't figured out why (there were some changes back and forth so I lost track).
Yes, ugly, and based on Muhammad's response, I plan on dropping this patch entirely, in fact.
Ah, finally I know why it sometimes worked and sometimes didn't ... yes, let's document that somehow.
Maybe we can even warn from the Makefile when the in-tree headers are not installed yet?
On 6/3/23 01:27, David Hildenbrand wrote:
On 03.06.23 00:20, John Hubbard wrote:
On 6/2/23 03:23, David Hildenbrand wrote: ...
Unfortunately, that seems to be the ugly way to handle this because including the in-tree headers seems to not work and I yet haven't figured out why (there were some changes back and forth so I lost track).
Yes, ugly, and based on Muhammad's response, I plan on dropping this patch entirely, in fact.
Ah, finally I know why it sometimes worked and sometimes didn't ... yes, let's document that somehow.
Maybe we can even warn from the Makefile when the in-tree headers are not installed yet?
Yes. And in fact, after some fooling around getting reacquainted with selftest Makefiles, it turns out that fussy part was setting up to detect the missing header files. And that seems to be working reliably now.
So from there, it's just as easy to automatically build any missing header files and continue as it is to warn about it.
I'll post a patch.
thanks,
On 6/2/23 6:33 AM, John Hubbard wrote:
UFFDIO_CONTINUE_MODE_WP, UFFD_FEATURE_WP_UNPOPULATED, USERFAULTFD_IOC, and USERFAULTFD_IOC_NEW are needed lately, but they are not in my host (Arch Linux) distro's userfaultfd.h yet. So put them in here.
Selftests are never supposed to build with native header files. Build the headers in kernel source first. Then building the selftests picks up these newly built headers by itself. The method to build header files has changed to `make headers`. The following command builds the mm selftests successfully every time for me.
make headers && make -C tools/testing/selftests/mm
Please let me know if this doesn't work for you. I'll try to reproduce and fix.
A better approach would be to include the uapi version of userfaultfd.h from the kernel tree, but that currently fails with rather difficult linker problems (__packed is defined multiple times, ugg), so defer that to another day and just fix the build for now.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/uffd-common.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index a1cdb78c0762..98847e41ecf9 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -36,6 +36,23 @@ #define UFFD_FLAGS (O_CLOEXEC | O_NONBLOCK | UFFD_USER_MODE_ONLY) +#ifndef UFFDIO_CONTINUE_MODE_WP +#define UFFDIO_CONTINUE_MODE_WP ((__u64)1<<1) +#endif
+#ifndef UFFD_FEATURE_WP_UNPOPULATED +#define UFFD_FEATURE_WP_UNPOPULATED (1<<13) +#endif
+/* ioctls for /dev/userfaultfd */ +#ifndef USERFAULTFD_IOC +#define USERFAULTFD_IOC 0xAA +#endif
+#ifndef USERFAULTFD_IOC_NEW +#define USERFAULTFD_IOC_NEW _IO(USERFAULTFD_IOC, 0x00) +#endif
#define _err(fmt, ...) \ do { \ int ret = errno; \
On 6/2/23 09:25, Muhammad Usama Anjum wrote:
On 6/2/23 6:33 AM, John Hubbard wrote:
UFFDIO_CONTINUE_MODE_WP, UFFD_FEATURE_WP_UNPOPULATED, USERFAULTFD_IOC, and USERFAULTFD_IOC_NEW are needed lately, but they are not in my host (Arch Linux) distro's userfaultfd.h yet. So put them in here.
Selftests are never supposed to build with native header files. Build the
Ah yes, I remember that now. Of course, the problem is that few people know or remember that, and it's undocumented as well.
headers in kernel source first. Then building the selftests picks up these newly built headers by itself. The method to build header files has changed to `make headers`. The following command builds the mm selftests successfully every time for me.
make headers && make -C tools/testing/selftests/mm
Please let me know if this doesn't work for you. I'll try to reproduce and fix.
Yes thanks. That's a pointer to a full solution, which needs to:
a) automatically invoke "make headers", at least for selftests/mm for now, and
b) Add something to perhaps Documentation/dev-tools/kselftest.rst to document this requirement.
I'll work on that.
thanks,
MADV_PAGEOUT, MADV_POPULATE_READ, MADV_COLLAPSE are conditionally defined as necessary. However, that was being done in .c files, and a new build failure came up that would have been automatically avoided had these been in a common header file.
So consolidate and move them all to vm_util.h, which fixes the build failure.
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/mm/cow.c | 7 ------- tools/testing/selftests/mm/khugepaged.c | 10 ---------- tools/testing/selftests/mm/vm_util.h | 10 ++++++++++ 3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index dc9d6fe86028..8882b05ec9c8 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -30,13 +30,6 @@ #include "../kselftest.h" #include "vm_util.h"
-#ifndef MADV_PAGEOUT -#define MADV_PAGEOUT 21 -#endif -#ifndef MADV_COLLAPSE -#define MADV_COLLAPSE 25 -#endif - static size_t pagesize; static int pagemap_fd; static size_t thpsize; diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c index 97adc0f34f9c..e88ee039d0eb 100644 --- a/tools/testing/selftests/mm/khugepaged.c +++ b/tools/testing/selftests/mm/khugepaged.c @@ -22,16 +22,6 @@
#include "vm_util.h"
-#ifndef MADV_PAGEOUT -#define MADV_PAGEOUT 21 -#endif -#ifndef MADV_POPULATE_READ -#define MADV_POPULATE_READ 22 -#endif -#ifndef MADV_COLLAPSE -#define MADV_COLLAPSE 25 -#endif - #define BASE_ADDR ((void *)(1UL << 30)) static unsigned long hpage_pmd_size; static unsigned long page_size; diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index 7f5aac0ac680..f04f82771cd0 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -41,3 +41,13 @@ unsigned long default_huge_page_size(void);
#define PAGEMAP_PRESENT(ent) (((ent) & (1ull << 63)) != 0) #define PAGEMAP_PFN(ent) ((ent) & ((1ull << 55) - 1)) + +#ifndef MADV_PAGEOUT +#define MADV_PAGEOUT 21 +#endif +#ifndef MADV_POPULATE_READ +#define MADV_POPULATE_READ 22 +#endif +#ifndef MADV_COLLAPSE +#define MADV_COLLAPSE 25 +#endif
On 02.06.23 03:33, John Hubbard wrote:
MADV_PAGEOUT, MADV_POPULATE_READ, MADV_COLLAPSE are conditionally defined as necessary. However, that was being done in .c files, and a new build failure came up that would have been automatically avoided had these been in a common header file.
So consolidate and move them all to vm_util.h, which fixes the build failure.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/cow.c | 7 ------- tools/testing/selftests/mm/khugepaged.c | 10 ---------- tools/testing/selftests/mm/vm_util.h | 10 ++++++++++ 3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index dc9d6fe86028..8882b05ec9c8 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -30,13 +30,6 @@ #include "../kselftest.h" #include "vm_util.h" -#ifndef MADV_PAGEOUT -#define MADV_PAGEOUT 21 -#endif -#ifndef MADV_COLLAPSE -#define MADV_COLLAPSE 25 -#endif
- static size_t pagesize; static int pagemap_fd; static size_t thpsize;
diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c index 97adc0f34f9c..e88ee039d0eb 100644 --- a/tools/testing/selftests/mm/khugepaged.c +++ b/tools/testing/selftests/mm/khugepaged.c @@ -22,16 +22,6 @@ #include "vm_util.h" -#ifndef MADV_PAGEOUT -#define MADV_PAGEOUT 21 -#endif -#ifndef MADV_POPULATE_READ -#define MADV_POPULATE_READ 22 -#endif -#ifndef MADV_COLLAPSE -#define MADV_COLLAPSE 25 -#endif
- #define BASE_ADDR ((void *)(1UL << 30)) static unsigned long hpage_pmd_size; static unsigned long page_size;
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index 7f5aac0ac680..f04f82771cd0 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -41,3 +41,13 @@ unsigned long default_huge_page_size(void); #define PAGEMAP_PRESENT(ent) (((ent) & (1ull << 63)) != 0) #define PAGEMAP_PFN(ent) ((ent) & ((1ull << 55) - 1))
+#ifndef MADV_PAGEOUT +#define MADV_PAGEOUT 21 +#endif +#ifndef MADV_POPULATE_READ +#define MADV_POPULATE_READ 22 +#endif +#ifndef MADV_COLLAPSE +#define MADV_COLLAPSE 25 +#endif
Reviewed-by: David Hildenbrand david@redhat.com
On 6/2/23 6:33 AM, John Hubbard wrote:
MADV_PAGEOUT, MADV_POPULATE_READ, MADV_COLLAPSE are conditionally defined as necessary. However, that was being done in .c files, and a new build failure came up that would have been automatically avoided had these been in a common header file.
There is a better way. Just like I've mentioned that building in-tree kernel headers and then mm selftests don't produce any build failures as they pick up in-tree compiled kernel header files. Don't move these definations to vm_util.h. Instead include asm-generic/mman-common.h which has all these definitions already. I've just removed what you have removed in this patch and include asm-generic/mman-common.h. It builds fine. But then there are more duplicated definition warnings.
So consolidate and move them all to vm_util.h, which fixes the build failure.
Signed-off-by: John Hubbard jhubbard@nvidia.com
tools/testing/selftests/mm/cow.c | 7 ------- tools/testing/selftests/mm/khugepaged.c | 10 ---------- tools/testing/selftests/mm/vm_util.h | 10 ++++++++++ 3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index dc9d6fe86028..8882b05ec9c8 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -30,13 +30,6 @@ #include "../kselftest.h" #include "vm_util.h" -#ifndef MADV_PAGEOUT -#define MADV_PAGEOUT 21 -#endif -#ifndef MADV_COLLAPSE -#define MADV_COLLAPSE 25 -#endif
static size_t pagesize; static int pagemap_fd; static size_t thpsize; diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c index 97adc0f34f9c..e88ee039d0eb 100644 --- a/tools/testing/selftests/mm/khugepaged.c +++ b/tools/testing/selftests/mm/khugepaged.c @@ -22,16 +22,6 @@ #include "vm_util.h" -#ifndef MADV_PAGEOUT -#define MADV_PAGEOUT 21 -#endif -#ifndef MADV_POPULATE_READ -#define MADV_POPULATE_READ 22 -#endif -#ifndef MADV_COLLAPSE -#define MADV_COLLAPSE 25 -#endif
#define BASE_ADDR ((void *)(1UL << 30)) static unsigned long hpage_pmd_size; static unsigned long page_size; diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index 7f5aac0ac680..f04f82771cd0 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -41,3 +41,13 @@ unsigned long default_huge_page_size(void); #define PAGEMAP_PRESENT(ent) (((ent) & (1ull << 63)) != 0) #define PAGEMAP_PFN(ent) ((ent) & ((1ull << 55) - 1))
+#ifndef MADV_PAGEOUT +#define MADV_PAGEOUT 21 +#endif +#ifndef MADV_POPULATE_READ +#define MADV_POPULATE_READ 22 +#endif +#ifndef MADV_COLLAPSE +#define MADV_COLLAPSE 25 +#endif
On 6/2/23 09:34, Muhammad Usama Anjum wrote:
On 6/2/23 6:33 AM, John Hubbard wrote:
MADV_PAGEOUT, MADV_POPULATE_READ, MADV_COLLAPSE are conditionally defined as necessary. However, that was being done in .c files, and a new build failure came up that would have been automatically avoided had these been in a common header file.
There is a better way. Just like I've mentioned that building in-tree kernel headers and then mm selftests don't produce any build failures as they pick up in-tree compiled kernel header files. Don't move these
OK.
definations to vm_util.h. Instead include asm-generic/mman-common.h which has all these definitions already. I've just removed what you have removed in this patch and include asm-generic/mman-common.h. It builds fine. But then there are more duplicated definition warnings.
umm, that's still a major problem. But let me look into it, based on your recommended approach of "make headers" first, thanks.
thanks,
On 02.06.23 03:33, John Hubbard wrote:
It turned out that an even dozen patches were required in order to get the selftests building cleanly, and all running, once again. I made it worse on myself by insisting on using clang, which seems to uncover a few more warnings than gcc these days.
So I still haven't gotten to my original goal of running a new HMM test that Alistair handed me (it's not here yet), but at least this fixes everything I ran into just now.
Thanks, now I can stop commenting out some (IIRC primarily uffd-related) tests when just wanting to run some specific tests on some of odd installations :D
I assume this round you only consideres 64bit builds, right? My last attempts with 32bit revealed a lot of issues, and I only fixed what I really wanted to test (COW).
On 6/2/23 02:32, David Hildenbrand wrote:
On 02.06.23 03:33, John Hubbard wrote:
It turned out that an even dozen patches were required in order to get the selftests building cleanly, and all running, once again. I made it worse on myself by insisting on using clang, which seems to uncover a few more warnings than gcc these days.
So I still haven't gotten to my original goal of running a new HMM test that Alistair handed me (it's not here yet), but at least this fixes everything I ran into just now.
Thanks, now I can stop commenting out some (IIRC primarily uffd-related) tests when just wanting to run some specific tests on some of odd installations :D
aha! :)
I assume this round you only consideres 64bit builds, right? My last attempts with 32bit revealed a lot of issues, and I only fixed what I really wanted to test (COW).
Yes, that's correct. I only tested on 64-bit. I've drifted into a "32 bit is mostly just for embedded systems" sort of mindset lately, and don't have anything set up for testing those locally.
thanks,
linux-kselftest-mirror@lists.linaro.org
-
David Hildenbrand -
John Hubbard -
Muhammad Usama Anjum -
Peter Xu