Make core VMA operations internal and testable

The vma files contain logic split from mmap.c for the most part and are all relevant to VMA logic, so maintain the same reviewers for both.

Signed-off-by Lorenzo Stoakes <[email protected]>

---
 MAINTAINERS | 13 +++++++++++++
 1 file changed, 13 insertions(+)

MAINTAINERS +13 -1

index 098d214f78d9..ff3e113ed081 100644
@@ -23971,6 +23971,19 @@ F:	include/uapi/linux/vsockmon.h
 F:	net/vmw_vsock/
 F:	tools/testing/vsock/
 
+VMA
+M:	Andrew Morton <[email protected]>
+R:	Liam R. Howlett <[email protected]>
+R:	Vlastimil Babka <[email protected]>
+R:	Lorenzo Stoakes <[email protected]>
+L:	[email protected]
+S:	Maintained
+W:	https://www.linux-mm.org
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
+F:	mm/vma.c
+F:	mm/vma.h
+F:	mm/vma_internal.h
+
 VMALLOC
 M:	Andrew Morton <[email protected]>
 R:	Uladzislau Rezki <[email protected]>
--

2.45.2

Quoted 2 lines · Lorenzo Stoakes

The vma files contain logic split from mmap.c for the most part and are all relevant to VMA logic, so maintain the same reviewers for both.

But it's still related to mmap.c a lot, so why not just rename that existing "MEMORY MAPPING" appropriately (how? haha) and expand it with the new files?

Quoted 32 lines

Signed-off-by Lorenzo Stoakes <[email protected]>

---
 MAINTAINERS | 13 +++++++++++++
 1 file changed, 13 insertions(+)

MAINTAINERS +13 -1

index 098d214f78d9..ff3e113ed081 100644
@@ -23971,6 +23971,19 @@ F:	include/uapi/linux/vsockmon.h
 F:	net/vmw_vsock/
 F:	tools/testing/vsock/
 
+VMA
+M:	Andrew Morton <[email protected]>
+R:	Liam R. Howlett <[email protected]>
+R:	Vlastimil Babka <[email protected]>
+R:	Lorenzo Stoakes <[email protected]>
+L:	[email protected]
+S:	Maintained
+W:	https://www.linux-mm.org
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
+F:	mm/vma.c
+F:	mm/vma.h
+F:	mm/vma_internal.h
+
 VMALLOC
 M:	Andrew Morton <[email protected]>
 R:	Uladzislau Rezki <[email protected]>
--

2.45.2

Establish a new userland VMA unit testing implementation under tools/testing which utilises existing logic providing maple tree support in userland utilising the now-shared code previously exclusive to radix tree testing.

This provides fundamental VMA operations whose API is defined in mm/vma.h, while stubbing out superfluous functionality.

This exists as a proof-of-concept, with the test implementation functional and sufficient to allow userland compilation of vma.c, but containing only cursory tests to demonstrate basic functionality.

Signed-off-by Lorenzo Stoakes <[email protected]>

---
 MAINTAINERS                            |   1 +
 include/linux/atomic.h                 |   2 +-
 include/linux/mmzone.h                 |   3 +-

 tools/testing/vma/.gitignore           |   6 +
 tools/testing/vma/Makefile             |  16 +
 tools/testing/vma/errors.txt           |   0
 tools/testing/vma/generated/autoconf.h |   2 +

 tools/testing/vma/linux/atomic.h       |  12 +
 tools/testing/vma/linux/mmzone.h       |  38 ++
 tools/testing/vma/vma.c                | 207 ++++++
 tools/testing/vma/vma_internal.h       | 882 +++++++++++++++++++++++++
 11 files changed, 1167 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/vma/.gitignore
 create mode 100644 tools/testing/vma/Makefile
 create mode 100644 tools/testing/vma/errors.txt
 create mode 100644 tools/testing/vma/generated/autoconf.h
 create mode 100644 tools/testing/vma/linux/atomic.h
 create mode 100644 tools/testing/vma/linux/mmzone.h
 create mode 100644 tools/testing/vma/vma.c
 create mode 100644 tools/testing/vma/vma_internal.h

MAINTAINERS +0 -0

index ff3e113ed081..c21099d0a123 100644

tools/testing/vma/.gitignore new file +3 -0

+idr.c
+radix-tree.c
+vma

tools/testing/vma/Makefile new file +6 -0

new file mode 100644
index 000000000000..70e728f2eee3
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+.PHONY: default
+
+default: vma
+

tools/testing/vma/linux/atomic.h new file +12 -0

new file mode 100644
index 000000000000..e01f66f98982
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _LINUX_ATOMIC_H
+#define _LINUX_ATOMIC_H
+
+#define atomic_t int32_t
+#define atomic_inc(x) uatomic_inc(x)
+#define atomic_read(x) uatomic_read(x)
+#define atomic_set(x, y) do {} while (0)
+#define U8_MAX UCHAR_MAX
+
+#endif	/* _LINUX_ATOMIC_H */

tools/testing/vma/linux/mmzone.h new file +0 -0

new file mode 100644
index 000000000000..e6a96c686610

tools/testing/vma/vma.c new file +52 -0

new file mode 100644
index 000000000000..1f32bc4d60c2
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "maple-shared.h"
+#include "vma_internal.h"
+
+/*
+ * Directly import the VMA implementation here. Our vma_internal.h wrapper
+ * provides userland-equivalent functionality for everything vma.c uses.
+ */
+#include "../../../mm/vma.c"
+
+const struct vm_operations_struct vma_dummy_vm_ops;
+
+#define ASSERT_TRUE(_expr)						\
+	do {								\
+		if (!(_expr)) {						\
+			fprintf(stderr,					\
+				"Assert FAILED at %s:%d:%s(): %s is FALSE.\n", \
+				__FILE__, __LINE__, __FUNCTION__, #_expr); \
+			return false;					\
+		}							\
+	} while (0)
+#define ASSERT_FALSE(_expr) ASSERT_TRUE(!(_expr))
+#define ASSERT_EQ(_val1, _val2) ASSERT_TRUE((_val1) == (_val2))
+#define ASSERT_NE(_val1, _val2) ASSERT_TRUE((_val1) != (_val2))
+
+static struct vm_area_struct *alloc_vma(struct mm_struct *mm,
+					unsigned long start,
+					unsigned long end,
+					pgoff_t pgoff,
+					vm_flags_t flags)
+{
+	struct vm_area_struct *ret = vm_area_alloc(mm);
+
+	if (ret == NULL)
+		return NULL;
+
+	ret->vm_start = start;
+	ret->vm_end = end;
+	ret->vm_pgoff = pgoff;
+	ret->__vm_flags = flags;
+
+	return ret;
+}
+
+static bool test_simple_merge(void)
+{
+	struct vm_area_struct *vma;

tools/testing/vma/vma.c new file +34 -0

+
+	vma = vma_merge_new_vma(&vmi, vma_left, vma_middle, 0x1000,
+				0x2000, 1);
+	ASSERT_NE(vma, NULL);
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x3000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+	ASSERT_EQ(vma->vm_flags, flags);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+static bool test_simple_modify(void)
+{
+	struct vm_area_struct *vma;
+	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
+	struct mm_struct mm = {};
+	struct vm_area_struct *init_vma = alloc_vma(&mm, 0, 0x3000, 0, flags);
+	VMA_ITERATOR(vmi, &mm, 0x1000);
+
+	ASSERT_FALSE(vma_link(&mm, init_vma));
+
+	/*
+	 * The flags will not be changed, the vma_modify_flags() function
+	 * performs the merge/split only.
+	 */
+	vma = vma_modify_flags(&vmi, init_vma, init_vma,
+			       0x1000, 0x2000, VM_READ | VM_MAYREAD);
+	ASSERT_NE(vma, NULL);
+	/* We modify the provided VMA, and on split allocate new VMAs. */

tools/testing/vma/vma.c new file +90 -0

+
+	vma_iter_set(&vmi, 0);
+	vma = vma_iter_load(&vmi);
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x1000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+
+	vm_area_free(vma);
+	vma_iter_clear(&vmi);
+
+	vma = vma_next(&vmi);
+
+	ASSERT_EQ(vma->vm_start, 0x1000);
+	ASSERT_EQ(vma->vm_end, 0x2000);
+	ASSERT_EQ(vma->vm_pgoff, 1);
+
+	vm_area_free(vma);
+	vma_iter_clear(&vmi);
+
+	vma = vma_next(&vmi);
+
+	ASSERT_EQ(vma->vm_start, 0x2000);
+	ASSERT_EQ(vma->vm_end, 0x3000);
+	ASSERT_EQ(vma->vm_pgoff, 2);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+static bool test_simple_expand(void)
+{
+	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
+	struct mm_struct mm = {};
+	struct vm_area_struct *vma = alloc_vma(&mm, 0, 0x1000, 0, flags);
+	VMA_ITERATOR(vmi, &mm, 0);
+
+	ASSERT_FALSE(vma_link(&mm, vma));
+
+	ASSERT_FALSE(vma_expand(&vmi, vma, 0, 0x3000, 0, NULL));
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x3000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+static bool test_simple_shrink(void)
+{
+	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
+	struct mm_struct mm = {};
+	struct vm_area_struct *vma = alloc_vma(&mm, 0, 0x3000, 0, flags);
+	VMA_ITERATOR(vmi, &mm, 0);
+
+	ASSERT_FALSE(vma_link(&mm, vma));
+
+	ASSERT_FALSE(vma_shrink(&vmi, vma, 0, 0x1000, 0));
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x1000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+int main(void)
+{
+	int num_tests = 0, num_fail = 0;
+
+	maple_tree_init();
+
+#define TEST(name)							\
+	do {								\
+		num_tests++;						\
+		if (!test_##name()) {					\
+			num_fail++;					\
+			fprintf(stderr, "Test " #name " FAILED\n");	\
+		}							\
+	} while (0)
+
+	TEST(simple_merge);

Hi Lorenzo,

Quoted 8 lines · On Wed, 3 Jul 2024 12:57:38 +0100 Lorenzo Stoakes…

Establish a new userland VMA unit testing implementation under tools/testing which utilises existing logic providing maple tree support in userland utilising the now-shared code previously exclusive to radix tree testing.

This provides fundamental VMA operations whose API is defined in mm/vma.h, while stubbing out superfluous functionality.

Quoted 3 lines

This exists as a proof-of-concept, with the test implementation functional and sufficient to allow userland compilation of vma.c, but containing only cursory tests to demonstrate basic functionality.

Overall, looks good to me. Appreciate this work. Nonetheless, I have some trivial questions and comments below.

Quoted 7 lines

Signed-off-by Lorenzo Stoakes <[email protected]>

---
 MAINTAINERS                            |   1 +
 include/linux/atomic.h                 |   2 +-
 include/linux/mmzone.h                 |   3 +-

I doubt if changes to above two files are intentional. Please read below comments.

Quoted 4 lines

 tools/testing/vma/.gitignore           |   6 +
 tools/testing/vma/Makefile             |  16 +
 tools/testing/vma/errors.txt           |   0
 tools/testing/vma/generated/autoconf.h |   2 +

I'm also unsure if above two files are intentionally added. Please read below comments.

Quoted 19 lines

 tools/testing/vma/linux/atomic.h       |  12 +
 tools/testing/vma/linux/mmzone.h       |  38 ++
 tools/testing/vma/vma.c                | 207 ++++++
 tools/testing/vma/vma_internal.h       | 882 +++++++++++++++++++++++++
 11 files changed, 1167 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/vma/.gitignore
 create mode 100644 tools/testing/vma/Makefile
 create mode 100644 tools/testing/vma/errors.txt
 create mode 100644 tools/testing/vma/generated/autoconf.h
 create mode 100644 tools/testing/vma/linux/atomic.h
 create mode 100644 tools/testing/vma/linux/mmzone.h
 create mode 100644 tools/testing/vma/vma.c
 create mode 100644 tools/testing/vma/vma_internal.h

MAINTAINERS +0 -0

index ff3e113ed081..c21099d0a123 100644

MAINTAINERS T: git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm · line 23986

Quoted 4 lines

MAINTAINERS +1 -0

@@ -23983,6 +23983,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
 F:	mm/vma.c
 F:	mm/vma.h
 F:	mm/vma_internal.h
+F:	tools/testing/vma/

Thank you for addressing my comment on the previous version :)

Btw, what do you think about moving the previous MAINTAINERS touching patch to the end of this patch series and making this change together at once?

Quoted 12 lines

MAINTAINERS +0 -0

 VMALLOC
 M:	Andrew Morton <[email protected]>

include/linux/atomic.h +0 -0

index 8dd57c3a99e9..badfba2fd10f 100644
@@ -81,4 +81,4 @@
 #include <linux/atomic/atomic-long.h>
 #include <linux/atomic/atomic-instrumented.h>
 

include/linux/atomic.h line 84

Quoted 2 lines

include/linux/atomic.h +1 -1

-#endif /* _LINUX_ATOMIC_H */
+#endif	/* _LINUX_ATOMIC_H */

Maybe unintended change?

include/linux/mmzone.h line 5

Quoted 12 lines

include/linux/mmzone.h +2 -1

index 41458892bc8a..30a22e57fa50 100644
@@ -1,4 +1,5 @@
-/* SPDX-License-Identifier: GPL-2.0 */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef _LINUX_MMZONE_H
 #define _LINUX_MMZONE_H
 

To my understanding, the test adds tools/testing/vma/linux/mmzone.h and uses it instead of this file. If I'm not missing something here, above license change may not really needed?

tools/testing/vma/.gitignore line 3

Quoted 9 lines

tools/testing/vma/.gitignore new file +3 -0

new file mode 100644
index 000000000000..d915f7d7fb1a
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+generated/bit-length.h
+generated/map-shift.h

I guess we should also have 'generated/autoconf.h' here? Please read below comment for the file, too.

Quoted 15 lines

tools/testing/vma/.gitignore new file +3 -0

+idr.c
+radix-tree.c
+vma

tools/testing/vma/Makefile new file +6 -0

new file mode 100644
index 000000000000..70e728f2eee3
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+.PHONY: default
+
+default: vma
+

tools/testing/vma/Makefile line 16

Quoted 10 lines

tools/testing/vma/Makefile new file +10 -0

+include ../shared/shared.mk
+
+OFILES = $(SHARED_OFILES) vma.o maple-shim.o
+TARGETS = vma
+
+vma:	$(OFILES) vma_internal.h ../../../mm/vma.c ../../../mm/vma.h
+	$(CC) $(CFLAGS) -o $@ $(OFILES) $(LDLIBS)
+
+clean:
+	$(RM) $(TARGETS) *.o radix-tree.c idr.c generated/map-shift.h generated/bit-length.h

If my assumption about generated/autoconf.h file is not wrong, I think we should also remove the file here, too. 'git' wouldn't care, but I think removing generated/ directory with files under it would be clearer for working space management.

tools/testing/vma/errors.txt

Quoted 3 lines

tools/testing/vma/errors.txt new file +0 -0

new file mode 100644
index 000000000000..e69de29bb2d1

I'm not seeing who is really using this empty file. Is this file intentionally added?

tools/testing/vma/generated/autoconf.h line 2

Quoted 9 lines

tools/testing/vma/generated/autoconf.h new file +2 -0

new file mode 100644
index 000000000000..92dc474c349b
@@ -0,0 +1,2 @@
+#include "bit-length.h"
+#define CONFIG_XARRAY_MULTI 1

Seems this file is automatically generated by ../shared/shared.mk. If I'm not wrong, I think removing this and adding changes I suggested to .gitignore and Makefile would be needed?

Since share.mk just copies the file while setting -I flag so that tools/testing/vma/vma.c can include files from share/ directory, maybe another option is simply including the file from the share/ directory without copying it here.

Also, the previous patch (tools: separate out shared radix-tree components) that adds this file at tools/testing/shared/ would need to add SPDX License identifier?

Quoted 23 lines

tools/testing/vma/linux/atomic.h new file +12 -0

new file mode 100644
index 000000000000..e01f66f98982
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _LINUX_ATOMIC_H
+#define _LINUX_ATOMIC_H
+
+#define atomic_t int32_t
+#define atomic_inc(x) uatomic_inc(x)
+#define atomic_read(x) uatomic_read(x)
+#define atomic_set(x, y) do {} while (0)
+#define U8_MAX UCHAR_MAX
+
+#endif	/* _LINUX_ATOMIC_H */

tools/testing/vma/linux/mmzone.h new file +0 -1

new file mode 100644
index 000000000000..e6a96c686610
--- /dev/null

tools/testing/vma/linux/mmzone.h line 1

Quoted 3 lines

+++ b/tools/testing/vma/linux/mmzone.h

tools/testing/vma/linux/mmzone.h new file +1 -0

@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */

I'm not very familiar with the license stuffs, but based on the changes to other files including that to include/linux/mmazone.h above, I was thinking this file would also need to update the license to GP-2.0-or-later. Should this be updated so?

[...]

Quoted 58 lines

tools/testing/vma/vma.c new file +52 -0

new file mode 100644
index 000000000000..1f32bc4d60c2
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "maple-shared.h"
+#include "vma_internal.h"
+
+/*
+ * Directly import the VMA implementation here. Our vma_internal.h wrapper
+ * provides userland-equivalent functionality for everything vma.c uses.
+ */
+#include "../../../mm/vma.c"
+
+const struct vm_operations_struct vma_dummy_vm_ops;
+
+#define ASSERT_TRUE(_expr)						\
+	do {								\
+		if (!(_expr)) {						\
+			fprintf(stderr,					\
+				"Assert FAILED at %s:%d:%s(): %s is FALSE.\n", \
+				__FILE__, __LINE__, __FUNCTION__, #_expr); \
+			return false;					\
+		}							\
+	} while (0)
+#define ASSERT_FALSE(_expr) ASSERT_TRUE(!(_expr))
+#define ASSERT_EQ(_val1, _val2) ASSERT_TRUE((_val1) == (_val2))
+#define ASSERT_NE(_val1, _val2) ASSERT_TRUE((_val1) != (_val2))
+
+static struct vm_area_struct *alloc_vma(struct mm_struct *mm,
+					unsigned long start,
+					unsigned long end,
+					pgoff_t pgoff,
+					vm_flags_t flags)
+{
+	struct vm_area_struct *ret = vm_area_alloc(mm);
+
+	if (ret == NULL)
+		return NULL;
+
+	ret->vm_start = start;
+	ret->vm_end = end;
+	ret->vm_pgoff = pgoff;
+	ret->__vm_flags = flags;
+
+	return ret;
+}
+
+static bool test_simple_merge(void)
+{
+	struct vm_area_struct *vma;

tools/testing/vma/vma.c line 62

Quoted 10 lines

tools/testing/vma/vma.c new file +10 -0

+	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
+	struct mm_struct mm = {};
+	struct vm_area_struct *vma_left = alloc_vma(&mm, 0, 0x1000, 0, flags);
+	struct vm_area_struct *vma_middle = alloc_vma(&mm, 0x1000, 0x2000, 1, flags);
+	struct vm_area_struct *vma_right = alloc_vma(&mm, 0x2000, 0x3000, 2, flags);
+	VMA_ITERATOR(vmi, &mm, 0x1000);
+
+	ASSERT_FALSE(vma_link(&mm, vma_left));
+	ASSERT_FALSE(vma_link(&mm, vma_middle));
+	ASSERT_FALSE(vma_link(&mm, vma_right));

So, vma_link() returns the error if failed, or zero, and therefore above assertions check if the function calls success as expected? It maybe too straighforward to people who familiar with the code, but I think adding some comment explaining the intent of the test would be nice for new comers.

IMHO, 'ASSERT_EQ(vma_link(...), 0)' may be easier to read.

Also, in case of assertion failures, the assertion prints the error and return false, to indicate the failure of the test, right? Then, would the memory allocated before, e.g., that for vma_{left,middle,right} above be leaked? I know this is just a test program in the user-space, but... If this is intentional, I think clarifying it somewhere would be nice.

Quoted 39 lines

tools/testing/vma/vma.c new file +38 -0

+
+	vma = vma_merge_new_vma(&vmi, vma_left, vma_middle, 0x1000,
+				0x2000, 1);
+	ASSERT_NE(vma, NULL);
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x3000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+	ASSERT_EQ(vma->vm_flags, flags);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+static bool test_simple_modify(void)
+{
+	struct vm_area_struct *vma;
+	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
+	struct mm_struct mm = {};
+	struct vm_area_struct *init_vma = alloc_vma(&mm, 0, 0x3000, 0, flags);
+	VMA_ITERATOR(vmi, &mm, 0x1000);
+
+	ASSERT_FALSE(vma_link(&mm, init_vma));
+
+	/*
+	 * The flags will not be changed, the vma_modify_flags() function
+	 * performs the merge/split only.
+	 */
+	vma = vma_modify_flags(&vmi, init_vma, init_vma,
+			       0x1000, 0x2000, VM_READ | VM_MAYREAD);
+	ASSERT_NE(vma, NULL);
+	/* We modify the provided VMA, and on split allocate new VMAs. */
+	ASSERT_EQ(vma, init_vma);
+
+	ASSERT_EQ(vma->vm_start, 0x1000);
+	ASSERT_EQ(vma->vm_end, 0x2000);

tools/testing/vma/vma.c line 106

Quoted 6 lines

tools/testing/vma/vma.c new file +6 -0

+	ASSERT_EQ(vma->vm_pgoff, 1);
+
+	/*
+	 * Now walk through the three split VMAs and make sure they are as
+	 * expected.
+	 */

I like these kind comments :)

Quoted 97 lines

tools/testing/vma/vma.c new file +96 -0

+
+	vma_iter_set(&vmi, 0);
+	vma = vma_iter_load(&vmi);
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x1000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+
+	vm_area_free(vma);
+	vma_iter_clear(&vmi);
+
+	vma = vma_next(&vmi);
+
+	ASSERT_EQ(vma->vm_start, 0x1000);
+	ASSERT_EQ(vma->vm_end, 0x2000);
+	ASSERT_EQ(vma->vm_pgoff, 1);
+
+	vm_area_free(vma);
+	vma_iter_clear(&vmi);
+
+	vma = vma_next(&vmi);
+
+	ASSERT_EQ(vma->vm_start, 0x2000);
+	ASSERT_EQ(vma->vm_end, 0x3000);
+	ASSERT_EQ(vma->vm_pgoff, 2);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+static bool test_simple_expand(void)
+{
+	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
+	struct mm_struct mm = {};
+	struct vm_area_struct *vma = alloc_vma(&mm, 0, 0x1000, 0, flags);
+	VMA_ITERATOR(vmi, &mm, 0);
+
+	ASSERT_FALSE(vma_link(&mm, vma));
+
+	ASSERT_FALSE(vma_expand(&vmi, vma, 0, 0x3000, 0, NULL));
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x3000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+static bool test_simple_shrink(void)
+{
+	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
+	struct mm_struct mm = {};
+	struct vm_area_struct *vma = alloc_vma(&mm, 0, 0x3000, 0, flags);
+	VMA_ITERATOR(vmi, &mm, 0);
+
+	ASSERT_FALSE(vma_link(&mm, vma));
+
+	ASSERT_FALSE(vma_shrink(&vmi, vma, 0, 0x1000, 0));
+
+	ASSERT_EQ(vma->vm_start, 0);
+	ASSERT_EQ(vma->vm_end, 0x1000);
+	ASSERT_EQ(vma->vm_pgoff, 0);
+
+	vm_area_free(vma);
+	mtree_destroy(&mm.mm_mt);
+
+	return true;
+}
+
+int main(void)
+{
+	int num_tests = 0, num_fail = 0;
+
+	maple_tree_init();
+
+#define TEST(name)							\
+	do {								\
+		num_tests++;						\
+		if (!test_##name()) {					\
+			num_fail++;					\
+			fprintf(stderr, "Test " #name " FAILED\n");	\
+		}							\
+	} while (0)
+
+	TEST(simple_merge);
+	TEST(simple_modify);
+	TEST(simple_expand);
+	TEST(simple_shrink);
+
+#undef TEST
+

tools/testing/vma/vma.c line 206

Quoted 4 lines

tools/testing/vma/vma.c new file +4 -0

+	printf("%d tests run, %d passed, %d failed.\n",
+	       num_tests, num_tests - num_fail, num_fail);
+
+	return EXIT_SUCCESS;

What do you think about making the return value indicates if the overall test has pass or failed, for easy integration with other test frameworks or scripts in future?

[...]

I didn't read all of this patch series in detail yet (I'm not sure if I'll have time to do that, so please don't wait for me), but looks nice work overall to me. Thank you for your efforts on this.

Thanks,
SJ

Kernel functionality is stubbed and shimmed as needed in tools/testing/vma/ which contains a fully functional userland vma_internal.h file and which imports mm/vma.c and mm/vma.h to be directly tested from userland.

 51 files changed, 3914 insertions(+), 2453 deletions(-)

Quoted 3 lines · On Wed, 3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes…

Kernel functionality is stubbed and shimmed as needed in tools/testing/vma/ which contains a fully functional userland vma_internal.h file and which imports mm/vma.c and mm/vma.h to be directly tested from userland.

Cool stuff.

Now we need to make sure that anyone who messes with vma code has run the tests. And has added more testcases, if appropriate.

Does it make sense to execute this test under selftests/ in some fashion? Quite a few people appear to be running the selftest code regularly and it would be good to make them run this as well.

Quoted 2 lines

 51 files changed, 3914 insertions(+), 2453 deletions(-)

eep. The best time for me to merge this is late in the -rc cycle so the large skew between mainline and mm.git doesn't spend months hampering ongoing development. But that merge time is right now.

Quoted 6 lines

Quoted 2 lines

 51 files changed, 3914 insertions(+), 2453 deletions(-)

eep. The best time for me to merge this is late in the -rc cycle so the large skew between mainline and mm.git doesn't spend months hampering ongoing development. But that merge time is right now.

Argh. Well, the numbers are scary, but it's _mostly_ moving code around with some pretty straightforward refactorings and adding a bunch of userland code that won't impact kernels at all.

So I'd argue this is less crazy in size than it might seem...

Quoted 7 lines · Andrew Morton

Quoted 3 lines · On Wed, 3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes…

Kernel functionality is stubbed and shimmed as needed in tools/testing/vma/ which contains a fully functional userland vma_internal.h file and which imports mm/vma.c and mm/vma.h to be directly tested from userland.

Cool stuff.

Thanks :)

Quoted 4 lines

Now we need to make sure that anyone who messes with vma code has run the tests. And has added more testcases, if appropriate.

Quoted 3 lines

Does it make sense to execute this test under selftests/ in some fashion? Quite a few people appear to be running the selftest code regularly and it would be good to make them run this as well.

I think it will be useful to do that, yes, but as the tests are currently a skeleton to both provide the stubbing out and to provide essentially an example of how you might test (though enough that it'd now be easy to add a _ton_ of tests), it's not quite ready to be run just yet.

Quoted 15 lines · On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <[email protected]>…

Quoted 7 lines · Andrew Morton

Quoted 3 lines · On Wed, 3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes…

Kernel functionality is stubbed and shimmed as needed in tools/testing/vma/ which contains a fully functional userland vma_internal.h file and which imports mm/vma.c and mm/vma.h to be directly tested from userland.

Cool stuff.

Thanks :)

Quoted 4 lines

Now we need to make sure that anyone who messes with vma code has run the tests. And has added more testcases, if appropriate.

Quoted 8 lines

Quoted 3 lines

Does it make sense to execute this test under selftests/ in some fashion? Quite a few people appear to be running the selftest code regularly and it would be good to make them run this as well.

I think it will be useful to do that, yes, but as the tests are currently a skeleton to both provide the stubbing out and to provide essentially an example of how you might test (though enough that it'd now be easy to add a _ton_ of tests), it's not quite ready to be run just yet.

If we will eventually move the files under selftests/, why dont' we place the files there from the beginning? Is there a strict rule saying files that not really involved with running tests or not ready cannot be added there? If so, could adding the files after the tests are ready to be run be an option? Cc-ing Shuah since I think she might have a comment.

Also, I haven't had enough time to read the patches in detail but just the cover letter a little bit. My humble impression from that is that this might better to eventually be kunit tests. I know there was a discussion with Kees on RFC v1 [1] which you kindly explained why you decide to implement this in user space. To my understanding, at least some of the problems are not real problems. For two things as examples,

1. I understand that you concern the test speed [2]. I think Kunit could be slower than the dedicated user space tests, but to my experience, it's not that bad when using the default UML-based execution.

2. My next humble undrestanding is that you want to test functions that you don't want to export [2,3] to kernel modules. To my understanding it's not limited on Kunit. I'm testing such DAMON functions using KUnit by including test code in the c file but protecting it via a config. For an example, please refer to DAMON_KUNIT_TEST.

I understand above are only small parts of the reason for your decision, and some of those would really unsupported by Kunit. In the case, I think adding this user space tests as is is good. Nonetheless, I think it would be good to hear some comments from Kunit developers. IMHO, letting them know the limitations will hopefully help setting their future TODO items. Cc-ing Brendan, David and Rae for that.

To recap, I have no strong opinions about this patch, but I think knowing how Selftests and KUnit developers think could be helpful.

[1] https://lore.kernel.org/202406270957.C0E5E8057@keescook
[2] https://lore.kernel.org/5zuowniex4sxy6l7erbsg5fiirf4d4f5fbpz2upay2igiwa2xk@vuezoh2wbqf4
[3] https://lore.kernel.org/[email protected]

Thanks,
SJ

[...]

Quoted 31 lines · SeongJae Park

Quoted 15 lines · On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <[email protected]>…

Quoted 7 lines · Andrew Morton

Quoted 3 lines · On Wed, 3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes…

Kernel functionality is stubbed and shimmed as needed in tools/testing/vma/ which contains a fully functional userland vma_internal.h file and which imports mm/vma.c and mm/vma.h to be directly tested from userland.

Cool stuff.

Thanks :)

Quoted 4 lines

Now we need to make sure that anyone who messes with vma code has run the tests. And has added more testcases, if appropriate.

Quoted 8 lines

Quoted 3 lines

Does it make sense to execute this test under selftests/ in some fashion? Quite a few people appear to be running the selftest code regularly and it would be good to make them run this as well.

I think it will be useful to do that, yes, but as the tests are currently a skeleton to both provide the stubbing out and to provide essentially an example of how you might test (though enough that it'd now be easy to add a _ton_ of tests), it's not quite ready to be run just yet.

If we will eventually move the files under selftests/, why dont' we place the files there from the beginning? Is there a strict rule saying files that not really involved with running tests or not ready cannot be added there? If so, could adding the files after the tests are ready to be run be an option? Cc-ing Shuah since I think she might have a comment.

My point to Andrew was that we could potentially automatically run these tests as part of a self-test run as they are so quick, at least in the future, if that made sense.

Quoted 8 lines

Also, I haven't had enough time to read the patches in detail but just the cover letter a little bit. My humble impression from that is that this might better to eventually be kunit tests. I know there was a discussion with Kees on RFC v1 [1] which you kindly explained why you decide to implement this in user space. To my understanding, at least some of the problems are not real problems. For two things as examples,

They are real problems. And I totally disagree that these should be kunit tests. I'm surprised you didn't find my and Liam's arguments compelling?

I suggest you try actually running tools/testing/vma/vma and putting a break point in gdb in vma_merge(), able to observe all state in great detail with no interrupts and see for yourself.

Quoted 4 lines

1. I understand that you concern the test speed [2]. I think Kunit could be slower than the dedicated user space tests, but to my experience, it's not that bad when using the default UML-based execution.

I'm sorry but running VMA code in the smallest possible form in userland is very clearly faster and you are missing the key point that we can _isolate_ anything we _don't need_.

There's no setup/teardown whatsoever, no clever tricks needed, we get to keep entirely internal interfaces internal and clean. It's compelling.

You are running the code as fast as you possibly can and that allows for lots of interesting things like being able to fuzz at scale, being able to run thousands of cases with basically zero setup/teardown or limits, etc. etc.

Also, it's basically impossible to explicitly _unit_ test vma merge and vma split and friends without invoking kernel stuff like TLB handling, MMU notifier, huge page handling, process setup/teardown, mm setup/teardown, rlimits, anon vma name handling, uprobes, memory policy handling, interval tree handling, lock contention, THP behaviour, etc. etc. etc.

With this test we can purely _unit_ test these fundamental operations, AND have the ability to for example in future - dump maple tree state from a buggy kernel situation that would result in a panic for instance - and recreate it immediately for debug.

We also then have the ability to have strong guarantees about the behaviour of these operations at a fundamental level.

If we want _system_ tests that bring in other kernel components then it makes more sense to use kunit/selftests. But this offers something else.

Quoted 2 lines

To recap, I have no strong opinions about this patch, but I think knowing how Selftests and KUnit developers think could be helpful.

With respect it strikes me that you have rather strong feelings on this. But again I make the plea that we don't hold this up on the basis of a debate about this vs. other options re: testing.

Kees was agreeable with this approach so I don't think we should really see too much objection to this.

The self tests differ from this and other tests using the userland-stubbed kernel approach in that they test system call invocation and assert expectations.

My point to Andrew was that we could potentially automatically run these tests as part of a self-test run as they are so quick, at least in the future, if that made sense.

Quoted 3 lines · On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <[email protected]>…

Quoted 2 lines · Andrew Morton

On Wed, 3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes <[email protected]> wrote:

Also, I haven't had enough time to read the patches in detail but just the cover letter a little bit. My humble impression from that is that this might better to eventually be kunit tests. I know there was a discussion with Kees on RFC v1 [1] which you kindly explained why you decide to implement this in user space. To my understanding, at least some of the problems are not real problems. For two things as examples,

1. I understand that you concern the test speed [2]. I think Kunit could be slower than the dedicated user space tests, but to my experience, it's not that bad when using the default UML-based execution.

2. My next humble undrestanding is that you want to test functions that you don't want to export [2,3] to kernel modules. To my understanding it's not limited on Kunit. I'm testing such DAMON functions using KUnit by including test code in the c file but protecting it via a config. For an example, please refer to DAMON_KUNIT_TEST.

I understand above are only small parts of the reason for your decision, and some of those would really unsupported by Kunit. In the case, I think adding this user space tests as is is good. Nonetheless, I think it would be good to hear some comments from Kunit developers. IMHO, letting them know the limitations will hopefully help setting their future TODO items. Cc-ing Brendan, David and Rae for that.

To recap, I have no strong opinions about this patch, but I think knowing how Selftests and KUnit developers think could be helpful.

Thanks, SJ.

While I'd love to have the VMA tests be KUnit tests (and there are several advantages, particularly for tooling and automation), I do think the more self-contained userspace tests are great in circumstances like this where the code is self-contained enough to make it possible. Ideally, we'd have some standards and helpers to make these consistent — kselftest and KUnit are both not quite perfect for this case — but I don't think we should hold up a useful set of changes so we can write a whole new framework.

(Personally, I think a userspace implementation of a subset of KUnit or a KUnit-like API would be useful, see below.)

Quoted 6 lines · On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <[email protected]>…

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Wed, 3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes…

[... snip ...]

Quoted 7 lines

Also, I haven't had enough time to read the patches in detail but just the cover letter a little bit. My humble impression from that is that this might better to eventually be kunit tests. I know there was a discussion with Kees on RFC v1 [1] which you kindly explained why you decide to implement this in user space. To my understanding, at least some of the problems are not real problems. For two things as examples,

Quoted 3 lines

1. I understand that you concern the test speed [2]. I think Kunit could be slower than the dedicated user space tests, but to my experience, it's not that bad when using the default UML-based execution.

KUnit/UML can be quite fast, but I do agree that a totally isolated test will be faster.

Quoted 8 lines

2. My next humble undrestanding is that you want to test functions that you don't want to export [2,3] to kernel modules. To my understanding it's not limited on Kunit. I'm testing such DAMON functions using KUnit by including test code in the c file but protecting it via a config. For an example, please refer to DAMON_KUNIT_TEST.

Quoted 6 lines

I understand above are only small parts of the reason for your decision, and some of those would really unsupported by Kunit. In the case, I think adding this user space tests as is is good. Nonetheless, I think it would be good to hear some comments from Kunit developers. IMHO, letting them know the limitations will hopefully help setting their future TODO items. Cc-ing Brendan, David and Rae for that.

There are a few different ways of working around this, including the '#include the source' method, and conditionally exporting symbols to a separate namespace (e.g., using VISIBLE_IF_KUNIT and EXPORT_SYMBOL_IF_KUNIT()).

Obviously, it's always going to be slightly nasty, but I don't think KUnit will fundamentally be uglier than any other similar hack.

Quoted 6 lines

To recap, I have no strong opinions about this patch, but I think knowing how Selftests and KUnit developers think could be helpful.

More generally, we've seen quite a few cases where we want to compile a small chunk of kernel code and some tests as a userspace binary, for a few different reasons, including:
- Improved speed/debuggability from being a "normal" userspace binary
- The desire to test userspace code which lives in the kernel tree (e.g., the perf tool)
- Smaller reproducable test cases to give to other parties (e.g., compiler developers)

So I think there's definitely a case for having these sorts of tests, it'd just be nice to be as consistent as we can. There are a few existing patches out there (most recently [1]) which implement a subset of the KUnit API in userspace, which has the twin advantages of making test code more consistent overall, and allowing some tests to be available both as KUnit tests and separate userspace tests (so we get the best of both worlds). Having a standard 'userspace kunit' implementation is definitely something I've thought about before, so I'll probably play around with that when I get some time.

Otherwise, if Shuah's okay with it, having these userspace tests be selftests seems at the very least an appropriate stopgap measure, which gets us some tooling and CI. I've always thought of selftests as "testing the running kernel", rather than the tree under test, but as long as it's clear that this is happening, there's no technical reason to avoid it,.

Cheers,
-- David

[1]: https://lore.kernel.org/all/[email protected]/