[PATCH 00/12] A minor flurry of selftest/mm fixes

[PATCH 00/12] A minor flurry of selftest/mm fixes

[John Hubbard]

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
-- 
2.40.1

[PATCH 01/12] selftests/mm: fix uffd-stress unused function warning

[John Hubbard]

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;
-- 
2.40.1

Re: [PATCH 01/12] selftests/mm: fix uffd-stress unused function warning

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

Re: [PATCH 01/12] selftests/mm: fix uffd-stress unused function warning

[Peter Xu]

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>

-- 
Peter Xu

[PATCH 02/12] selftests/mm: fix unused variable warning in hugetlb-madvise.c

[John Hubbard]

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++)
-- 
2.40.1

Re: [PATCH 02/12] selftests/mm: fix unused variable warning in hugetlb-madvise.c

[David Hildenbrand]

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).

-- 
Thanks,

David / dhildenb

Re: [PATCH 02/12] selftests/mm: fix unused variable warning in hugetlb-madvise.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 03/12] selftests/mm: fix unused variable warning in migration.c

[John Hubbard]

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) {
-- 
2.40.1

Re: [PATCH 03/12] selftests/mm: fix unused variable warning in migration.c

[David Hildenbrand]

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.

-- 
Thanks,

David / dhildenb

Re: [PATCH 03/12] selftests/mm: fix unused variable warning in migration.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

[John Hubbard]

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);
-- 
2.40.1

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

[David Hildenbrand]

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;
}

-- 
Thanks,

David / dhildenb

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

[Peter Xu]

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;
> }

I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot
since the sscanf() just worked..

-- 
Peter Xu

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

[Peter Xu]

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

-- 
Peter Xu

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 05/12] selftests/mm: fix invocation of tests that are run via shell scripts

[John Hubbard]

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
-- 
2.40.1

Re: [PATCH 05/12] selftests/mm: fix invocation of tests that are run via shell scripts

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

Re: [PATCH 05/12] selftests/mm: fix invocation of tests that are run via shell scripts

[John Hubbard]

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.

-- 
John Hubbard
NVIDIA

Re: [PATCH 05/12] selftests/mm: fix invocation of tests that are run via shell scripts

[Peter Xu]

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.

-- 
Peter Xu

Re: [PATCH 05/12] selftests/mm: fix invocation of tests that are run via shell scripts

[John Hubbard]

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-default-shell-from-bash-to-zsh/

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 05/12] selftests/mm: fix invocation of tests that are run via shell scripts

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH 05/12] selftests/mm: fix invocation of tests that are run via shell scripts

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 06/12] selftests/mm: .gitignore: add mkdirty, va_high_addr_switch

[John Hubbard]

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
-- 
2.40.1

Re: [PATCH 06/12] selftests/mm: .gitignore: add mkdirty, va_high_addr_switch

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

[PATCH 07/12] selftests/mm: set -Wno-format-security to avoid uffd build warnings

[John Hubbard]

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)
 
-- 
2.40.1

Re: [PATCH 07/12] selftests/mm: set -Wno-format-security to avoid uffd build warnings

[David Hildenbrand]

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]
...

-- 
Thanks,

David / dhildenb

Re: [PATCH 07/12] selftests/mm: set -Wno-format-security to avoid uffd build warnings

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 08/12] selftests/mm: fix a "possibly uninitialized" warning in pkey-x86.h

[John Hubbard]

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;
 
-- 
2.40.1

Re: [PATCH 08/12] selftests/mm: fix a "possibly uninitialized" warning in pkey-x86.h

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

[PATCH 09/12] selftests/mm: move psize(), pshift() into vm_utils.c

[John Hubbard]

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);
-- 
2.40.1

Re: [PATCH 09/12] selftests/mm: move psize(), pshift() into vm_utils.c

[David Hildenbrand]

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.

-- 
Thanks,

David / dhildenb

Re: [PATCH 09/12] selftests/mm: move psize(), pshift() into vm_utils.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[John Hubbard]

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
  */
-- 
2.40.1

Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[Peter Xu]

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.

-- 
Peter Xu

Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 11/12] selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures

[John Hubbard]

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;				\
-- 
2.40.1

Re: [PATCH 11/12] selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

Re: [PATCH 11/12] selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 11/12] selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures

[David Hildenbrand]

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?

-- 
Cheers,

David / dhildenb

Re: [PATCH 11/12] selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 11/12] selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures

[Muhammad Usama Anjum]

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;				\

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 11/12] selftests/mm: fix missing UFFDIO_CONTINUE_MODE_WP and similar build failures

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 12/12] selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE

[John Hubbard]

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
-- 
2.40.1

Re: [PATCH 12/12] selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

Re: [PATCH 12/12] selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE

[Muhammad Usama Anjum]

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

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 12/12] selftests/mm: fix uffd-unit-tests.c build failure due to missing MADV_COLLAPSE

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 00/12] A minor flurry of selftest/mm fixes

[David Hildenbrand]

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).

-- 
Thanks,

David / dhildenb

Re: [PATCH 00/12] A minor flurry of selftest/mm fixes

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA