by David Hildenbrand

[permalink] [raw]

Subject: Re: selftests/vm madv_populate.c test

On 15.10.21 18:40, Shuah Khan wrote:
> On 10/15/21 10:34 AM, David Hildenbrand wrote:
>> On 15.10.21 18:28, David Hildenbrand wrote:
>>> On 15.10.21 18:25, Shuah Khan wrote:
>>>> 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?
>>>
>>> I just double-checked (after make clean) and there is still something
>>> wrong :( the only think that seems to work is the
>>>
>>> +#include "../../../../usr/include/linux/mman.h"
>>> #include <sys/mman.h>
>>>
>>> hack.
>>>
>>> Using "-nostdinc" won't work because we need other headers :(
>>>
>>
>> And ... I think I know the problem.
>>
>> In ../../../../usr/include, there is no "sys" directory. It's called
>> "linux".
>>
>> But including <linux/mman.h> instead of <sys/mman.h> doesn't work
>> either. The only thing that seems to work is
>>
>>
>> diff --git a/tools/testing/selftests/vm/madv_populate.c
>> b/tools/testing/selftests/vm/madv_populate.c
>> index b959e4ebdad4..3ee0e8275600 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 <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) */
>>
>>
>
> I tried with just the following and it worked after kselftest-clean
> as well.
>
> diff --git a/tools/testing/selftests/vm/madv_populate.c b/tools/testing/selftests/vm/madv_populate.c
> index b959e4ebdad4..f9e4b8e1b28c 100644
> --- a/tools/testing/selftests/vm/madv_populate.c
> +++ b/tools/testing/selftests/vm/madv_populate.c
> @@ -14,6 +14,7 @@
> #include <unistd.h>
> #include <errno.h>
> #include <fcntl.h>
> +#include <linux/mman.h>
> #include <sys/mman.h>
>
> #include "../kselftest.h"

I'll send a patch, thanks. (I hope this combination won't cause trouble
in the future)

--
Thanks,

David / dhildenb