Message-Id: <20240704055956.96925-1-sj@kernel.org>
Date: Wed,  3 Jul 2024 22:59:56 -0700
From: SeongJae Park <sj@...nel.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: SeongJae Park <sj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	"Liam R . Howlett" <Liam.Howlett@...cle.com>,
	Vlastimil Babka <vbabka@...e.cz>,
	Matthew Wilcox <willy@...radead.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>,
	Jan Kara <jack@...e.cz>,
	Eric Biederman <ebiederm@...ssion.com>,
	Kees Cook <kees@...nel.org>,
	Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [PATCH 7/7] tools: add skeleton code for userland testing of VMA logic

Hi Lorenzo,

On Wed,  3 Jul 2024 12:57:38 +0100 Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:

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

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

> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> ---
>  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.

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

>  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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ff3e113ed081..c21099d0a123 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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?

> 
>  VMALLOC
>  M:	Andrew Morton <akpm@...ux-foundation.org>
> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index 8dd57c3a99e9..badfba2fd10f 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -81,4 +81,4 @@
>  #include <linux/atomic/atomic-long.h>
>  #include <linux/atomic/atomic-instrumented.h>
> 
> -#endif /* _LINUX_ATOMIC_H */
> +#endif	/* _LINUX_ATOMIC_H */

Maybe unintended change?

> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 41458892bc8a..30a22e57fa50 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -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?

> diff --git a/tools/testing/vma/.gitignore b/tools/testing/vma/.gitignore
> new file mode 100644
> index 000000000000..d915f7d7fb1a
> --- /dev/null
> +++ b/tools/testing/vma/.gitignore
> @@ -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.

> +idr.c
> +radix-tree.c
> +vma
> diff --git a/tools/testing/vma/Makefile b/tools/testing/vma/Makefile
> new file mode 100644
> index 000000000000..70e728f2eee3
> --- /dev/null
> +++ b/tools/testing/vma/Makefile
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +.PHONY: default
> +
> +default: vma
> +
> +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.

> diff --git a/tools/testing/vma/errors.txt b/tools/testing/vma/errors.txt
> new file mode 100644
> index 000000000000..e69de29bb2d1

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

> diff --git a/tools/testing/vma/generated/autoconf.h b/tools/testing/vma/generated/autoconf.h
> new file mode 100644
> index 000000000000..92dc474c349b
> --- /dev/null
> +++ b/tools/testing/vma/generated/autoconf.h
> @@ -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?

> diff --git a/tools/testing/vma/linux/atomic.h b/tools/testing/vma/linux/atomic.h
> new file mode 100644
> index 000000000000..e01f66f98982
> --- /dev/null
> +++ b/tools/testing/vma/linux/atomic.h
> @@ -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 */
> diff --git a/tools/testing/vma/linux/mmzone.h b/tools/testing/vma/linux/mmzone.h
> new file mode 100644
> index 000000000000..e6a96c686610
> --- /dev/null
> +++ b/tools/testing/vma/linux/mmzone.h
> @@ -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?

[...]
> diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
> new file mode 100644
> index 000000000000..1f32bc4d60c2
> --- /dev/null
> +++ b/tools/testing/vma/vma.c
> @@ -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;
> +	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.

> +
> +	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);
> +	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 :)

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

Powered by blists - more mailing lists