Re: selftests/vm madv_populate.c test

[Shuah Khan <skhan@linuxfoundation.org>]

On 10/15/21 10:19 AM, David Hildenbrand wrote:
> On 15.10.21 18:15, David Hildenbrand wrote:
>> On 15.10.21 18:06, David Hildenbrand wrote:
>>> On 15.10.21 17:47, David Hildenbrand wrote:
>>>> On 15.10.21 17:45, Shuah Khan wrote:
>>>>> On 9/18/21 1:41 AM, David Hildenbrand wrote:
>>>>>> On 18.09.21 00:45, Shuah Khan wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> I am running into the following warning when try to build this test:
>>>>>>>
>>>>>>> madv_populate.c:334:2: warning: #warning "missing MADV_POPULATE_READ or MADV_POPULATE_WRITE definition" [-Wcpp]
>>>>>>>      334 | #warning "missing MADV_POPULATE_READ or MADV_POPULATE_WRITE definition"
>>>>>>>          |  ^~~~~~~
>>>>>>>
>>>>>>>
>>>>>>> I see that the following handling is in place. However there is no
>>>>>>> other information to explain why the check is necessary.
>>>>>>>
>>>>>>> #if defined(MADV_POPULATE_READ) && defined(MADV_POPULATE_WRITE)
>>>>>>>
>>>>>>> #else /* defined(MADV_POPULATE_READ) && defined(MADV_POPULATE_WRITE) */
>>>>>>>
>>>>>>> #warning "missing MADV_POPULATE_READ or MADV_POPULATE_WRITE definition"
>>>>>>>
>>>>>>> I do see these defined in:
>>>>>>>
>>>>>>> include/uapi/asm-generic/mman-common.h:#define MADV_POPULATE_READ       22
>>>>>>> include/uapi/asm-generic/mman-common.h:#define MADV_POPULATE_WRITE      23
>>>>>>>
>>>>>>> Is this the case of missing include from madv_populate.c?
>>>>>>
>>>>>> Hi Shuan,
>>>>>>
>>>>>> note that we're including "#include <sys/mman.h>", which in my
>>>>>> understanding maps to the version installed on your system instead
>>>>>> of the one in our build environment.ing.
>>>>>>
>>>>>> So as soon as you have a proper kernel + the proper headers installed
>>>>>> and try to build, it would pick up MADV_POPULATE_READ and
>>>>>> MADV_POPULATE_WRITE from the updated headers. That makes sense: you
>>>>>> annot run any MADV_POPULATE_READ/MADV_POPULATE_WRITE tests on a kernel
>>>>>> that doesn't support it.
>>>>>>
>>>>>> See vm/userfaultfd.c where we do something similar.
>>>>>>
>>>>>
>>>>> Kselftest is for testing the kernel with kernel headers. That is the
>>>>> reason why there is the dependency on header install.
>>>>>
>>>>>>
>>>>>> As soon as we have a proper environment, it seems to work just fine:
>>>>>>
>>>>>> Linux vm-0 5.15.0-0.rc1.20210915git3ca706c189db.13.fc36.x86_64 #1 SMP Thu Sep 16 11:32:54 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
>>>>>> [root@vm-0 linux]# cat /etc/redhat-release
>>>>>> Fedora release 36 (Rawhide)
>>>>>
>>>>> This is a distro release. We don't want to have dependency on headers
>>>>> from the distro to run selftests. Hope this makes sense.
>>>>>
>>>>> I still see this on my test system running Linux 5.15-rc5.
>>>>
>>>> Did you also install Linux headers? I assume no, correct?
>>>>
>>>
>>> What happens in your environment when compiling and running the
>>> memfd_secret test?
>>>
>>> If assume you'll see a "skip" when executing, because it might also
>>> refer to the local version of linux headers and although it builds, it
>>> really cannot build something "functional". It just doesn't add a
>>> "#warning" to make that obvious.
>>>
>>
>> The following works but looks extremely hackish.
>>
>> diff --git a/tools/testing/selftests/vm/madv_populate.c
>> b/tools/testing/selftests/vm/madv_populate.c
>> index b959e4ebdad4..ab26163db540 100644
>> --- a/tools/testing/selftests/vm/madv_populate.c
>> +++ b/tools/testing/selftests/vm/madv_populate.c
>> @@ -14,12 +14,11 @@
>>   #include <unistd.h>
>>   #include <errno.h>
>>   #include <fcntl.h>
>> +#include "../../../../usr/include/linux/mman.h"
>>   #include <sys/mman.h>
>>
>>   #include "../kselftest.h"
>>
>> -#if defined(MADV_POPULATE_READ) && defined(MADV_POPULATE_WRITE)
>> -
>>   /*
>>    * For now, we're using 2 MiB of private anonymous memory for all tests.
>>    */
>> @@ -328,15 +327,3 @@ int main(int argc, char **argv)
>>                                     err, ksft_test_num());
>>          return ksft_exit_pass();
>>   }
>> -
>> -#else /* defined(MADV_POPULATE_READ) && defined(MADV_POPULATE_WRITE) */
>> -
>> -#warning "missing MADV_POPULATE_READ or MADV_POPULATE_WRITE definition"
>> -
>> -int main(int argc, char **argv)
>> -{
>> -       ksft_print_header();
>> -       ksft_exit_skip("MADV_POPULATE_READ or MADV_POPULATE_WRITE not
>> defined\n");
>> -}
>> -
>> -#endif /* defined(MADV_POPULATE_READ) && defined(MADV_POPULATE_WRITE) */
>>
>>
>> There has to be some clean way to achieve the same.
>>
> 
> Sorry for the spam,
> 
> diff --git a/tools/testing/selftests/vm/Makefile
> b/tools/testing/selftests/vm/Makefile
> index d9605bd10f2d..ce198b329ff5 100644
> --- a/tools/testing/selftests/vm/Makefile
> +++ b/tools/testing/selftests/vm/Makefile
> @@ -23,7 +23,7 @@ MACHINE ?= $(shell echo $(uname_M) | sed -e
> 's/aarch64.*/arm64/' -e 's/ppc64.*/p
>   # LDLIBS.
>   MAKEFLAGS += --no-builtin-rules
> 
> -CFLAGS = -Wall -I ../../../../usr/include $(EXTRA_CFLAGS)
> +CFLAGS = -Wall -idirafter ../../../../usr/include $(EXTRA_CFLAGS)
>   LDLIBS = -lrt -lpthread
>   TEST_GEN_FILES = compaction_test
>   TEST_GEN_FILES += gup_test
> 
> 
> Seems to set the right include path priority.
> 
> 

Yes. It works on linux-next-20211012

Do you mind sending a me patch for this?

thanks,
-- Shuah