Re: + selftests-mm-define-madv_pageout-to-fix-compilation-issues.patch added to mm-unstable branch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/10/23 12:56, David Hildenbrand wrote:
On 10.01.23 12:52, Mirsad Todorovac wrote:


On 1/10/23 12:18, David Hildenbrand wrote:
On 10.01.23 12:00, Mirsad Todorovac wrote:
Hi,

On 1/10/23 00:36, Andrew Morton wrote:
The patch titled
        Subject: selftests/mm: define MADV_PAGEOUT to fix compilation
issues
has been added to the -mm mm-unstable branch.  Its filename is
        selftests-mm-define-madv_pageout-to-fix-compilation-issues.patch

This patch will shortly appear at
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/selftests-mm-define-madv_pageout-to-fix-compilation-issues.patch

This patch will later appear in the mm-unstable branch at
       git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm

Before you just go and hit "reply", please:
      a) Consider who else should be cc'ed
      b) Prefer to cc a suitable mailing list as well
      c) Ideally: find the original patch on the mailing list and do a
         reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/process/submit-checklist.rst when
testing your code ***

The -mm tree is included into linux-next via the mm-everything
branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
and is updated there every 2-3 working days

------------------------------------------------------
From: David Hildenbrand <david@xxxxxxxxxx>
Subject: selftests/mm: define MADV_PAGEOUT to fix compilation issues
Date: Mon, 9 Jan 2023 18:12:55 +0100

If MADV_PAGEOUT is not defined (e.g., on AlmaLinux 8), compilation will
fail.  Let's fix that like khugepaged.c does by conditionally defining
MADV_PAGEOUT.

Link: https://lkml.kernel.org/r/20230109171255.488749-1-david@xxxxxxxxxx
Fixes: 69c66add5663 ("selftests/vm: anon_cow: test COW handling of
anonymous memory")
Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@xxxxxxxxxxxx>
Cc: Shuah Khan <shuah@xxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---

    tools/testing/selftests/mm/cow.c |    3 +++
    1 file changed, 3 insertions(+)

---
a/tools/testing/selftests/mm/cow.c~selftests-mm-define-madv_pageout-to-fix-compilation-issues
+++ a/tools/testing/selftests/mm/cow.c
@@ -30,6 +30,9 @@
    #include "../kselftest.h"
    #include "vm_util.h"
+#ifndef MADV_PAGEOUT
+#define MADV_PAGEOUT 21
+#endif
    #ifndef MADV_COLLAPSE
    #define MADV_COLLAPSE 25
    #endif
_

Patches currently in -mm which might be from david@xxxxxxxxxx are

mm-hugetlb-fix-pte-marker-handling-in-hugetlb_change_protection.patch
mm-hugetlb-fix-uffd-wp-handling-for-migration-entries-in-hugetlb_change_protection.patch
mm-userfaultfd-enable-writenotify-while-userfaultfd-wp-is-enabled-for-a-vma.patch
mm-userfaultfd-rely-on-vma-vm_page_prot-in-uffd_wp_range.patch
mm-userfaultfd-rely-on-vma-vm_page_prot-in-uffd_wp_range-fix.patch
mm-mprotect-drop-pgprot_t-parameter-from-change_protection.patch
mm-mprotect-drop-pgprot_t-parameter-from-change_protection-fix.patch
selftests-vm-cow-add-cow-tests-for-collapsing-of-pte-mapped-anon-thp.patch
mm-nommu-factor-out-check-for-nommu-shared-mappings-into-is_nommu_shared_mapping.patch
mm-nommu-dont-use-vm_mayshare-for-map_private-mappings.patch
drivers-misc-open-dice-dont-touch-vm_mayshare.patch
selftests-mm-define-madv_pageout-to-fix-compilation-issues.patch
Please consider alternative patch instead. I am submitting it for a
review.

It would give the referential integrity to the source of the selftest/vm
subtree.

#ifdef ... #endif encapsulation around `#define __USE_GNU` eliminates a
warning.

RATIONALE:

Though it is ulikely that the values of MADV_PAGEOUT,
MADV_POPULATE_READ and MADV_COLLAPSE will change, the source integrity
would benefit from having it changed in only one place,
.../usr/include/asm-generic/mman-common.h, included by >
#include <linux/mman.h>

Would that imply that we're taking the in-tree sources and not the
installed kernel headers?

Because compilation would fail if the installed kernel headers don't
define these.

It seems that userfaultfd.c already included <linux/mman.h>,
so nothing is lost or gained in compilation successes, except that
things look more uniform, coherent and consistent?

I included it in tools/testing/selftests/mm/madv_populate.c as well BUT that was
not sufficient to make it compile in all environments.

Hi, David,

It's unobvious why, for (as you can witness from the source)

#include <linux/mman.h> includes
#include <asm/mman.h> includes
#include <asm-generic/mman.h> which in turn includes
#include <asm-generic/mman-common.h>

that defines all of the

#define MADV_PAGEOUT    21              /* reclaim these pages */
#define MADV_POPULATE_READ      22      /* populate (prefault) page tables readable */
#define MADV_POPULATE_WRITE     23      /* populate (prefault) page tables writable */

So, everything is within the in-tree includes, so I can't tell what went wrong.

I see that "#include <linux/magic.h>" is already used.

BTW, did userfaultfd.c compile on that Debian 11.5 environment?

commit d88825f22b8f1cad8acb429b6bf0a54c85d7b93f
Author: David Hildenbrand <david@xxxxxxxxxx>
Date:   Mon Dec 5 20:37:14 2022 +0100

     selftests/vm: madv_populate: fix missing MADV_POPULATE_(READ|WRITE) definitions
     The tests fail to compile in some environments (e.g., Debian 11.5 on x86).

On Debian 11 on x86_64 I had no issues (just checked). (Your fix applied.)

Only MADV_PAGEOUT is defined in /usr/include:

mtodorov@magrf:~$ grep -E '(MADV_PAGEOUT|MADV_POPULATE_READ|MADV_POPULATE_WRITE)' -r /usr/include
/usr/include/x86_64-linux-gnu/bits/mman-linux.h:# define MADV_PAGEOUT     21    /* Reclaim these pages.  */
/usr/include/asm-generic/mman-common.h:#define MADV_PAGEOUT     21              /* reclaim these pages */
mtodorov@magrf:~$

However, "memfd-secret.c" and "mlock-random-test.c" fail to include <sys/capability.h>
despite being in both /usr/include/linux/capability.h and ../../../../usr/include/sys/capability.h:

mtodorov@magrf:~/linux/kernel/linux_torvalds/tools/testing/selftests/vm$ make -k
gcc -Wall -I /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/../../.. -I /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/../../../usr/include -isystem /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/../../../usr/include -no-pie memfd_secret.c -lrt -lpthread -lcap -o /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/vm/memfd_secret
memfd_secret.c:16:10: fatal error: sys/capability.h: No such file or directory
   16 | #include <sys/capability.h>
      |          ^~~~~~~~~~~~~~~~~~
compilation terminated.
make: *** [../lib.mk:145: /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/vm/migration] Error 1
gcc -Wall -I /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/../../.. -I /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/../../../usr/include -isystem /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/../../../usr/include -no-pie mlock-random-test.c -lrt -lpthread -lcap -o /home/admin/mtodorov/linux/kernel/linux_torvalds/tools/testing/selftests/vm/mlock-random-test
mlock-random-test.c:8:10: fatal error: sys/capability.h: No such file or directory
    8 | #include <sys/capability.h>
      |          ^~~~~~~~~~~~~~~~~~
compilation terminated.

usr/include/linux/capability.h is existing, but the compiler somehow fails to find it:

mtodorov@magrf:~/linux/kernel/linux_torvalds/tools/testing/selftests/vm$ grep sys/capability.h *.c
memfd_secret.c:#include <sys/capability.h>
mlock-random-test.c:#include <sys/capability.h>
mtodorov@magrf:~/linux/kernel/linux_torvalds/tools/testing/selftests/vm$ find /usr/include -name capability.h
/usr/include/linux/capability.h
mtodorov@magrf:~/linux/kernel/linux_torvalds/tools/testing/selftests/vm$

     Let's simply conditionally define MADV_POPULATE_(READ|WRITE) if not
     already defined, similar to how the khugepaged.c test handles it.
     Link: https://lkml.kernel.org/r/20221205193716.276024-3-david@xxxxxxxxxx
     Fixes: 39b2e5cae43d ("selftests/vm: make MADV_POPULATE_(READ|WRITE) use in-tree headers")
     Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
     Cc: Shuah Khan <shuah@xxxxxxxxxx>
     Cc: Yang Li <yang.lee@xxxxxxxxxxxxxxxxx>
     Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>

IMHO, I don't have your experience and knowledge, but from the fresh perspective,
kernel will bring in its own selftest suite all the defines in <linux/mman.h>
that are supported in the particular version.

AFAIK, testing with different version of kernel is not recommended.

Of course, we can easily test with "gcc -E" what exactly gets included.

We don't seem to have a uniform way of handling MADV_PAGEOUT and
MADV_POPULATE(READ|WRITE) even within different C sources within
tools/testing/selftests/vm itself, so it means that we spread the spectrum
of systems and compilations that we will break. :-/

To repeat what is obvious, I am not an authority on the Linux kernel,
but as I said, I have the benefit of a fresh perspective ...

There was an open source proverb that "more eyes see more" :)

Thanks,
Mirsad

--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia



[Index of Archives]     [Kernel Archive]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]

  Powered by Linux