[PATCH 0/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[PATCH 0/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Brendan Jackman]

De-duplicating this lets us delete a bit of code. 

Ulterior motive: I'm working on a new set of the userspace-based unit
tests, which will need the atomics API too. That would involve even more
duplication, so while the win in this patchset alone is very minimal, it
looks a lot more significant with my other WIP patchset.

I've tested these commands:

make -C tools/testing/vma -j 
tools/testing/vma/vma

make -C tools/testing/radix-tree -j
tools/testing/radix-tree/main

Note the EXTRA_CFLAGS patch is actually orthogonal, let me know if you'd
prefer I send it separately.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
Brendan Jackman (3):
      tools: testing: Allow importing arch headers in shared.mk
      tools: testing: Use existing atomic.h for vma/radix-tree tests
      tools: testing: Support EXTRA_CFLAGS in shared.mk

 tools/testing/shared/linux/maple_tree.h |  6 ++----
 tools/testing/shared/shared.mk          |  6 +++++-
 tools/testing/vma/linux/atomic.h        | 17 -----------------
 tools/testing/vma/vma_internal.h        |  3 ++-
 4 files changed, 9 insertions(+), 23 deletions(-)
---
base-commit: fab1beda7597fac1cecc01707d55eadb6bbe773c
change-id: 20250827-b4-vma-no-atomic-h-0f8ebc2fe4f9

Best regards,
-- 
Brendan Jackman <jackmanb@google.com>

[PATCH 1/3] tools: testing: Allow importing arch headers in shared.mk

[Brendan Jackman]

There is an arch/ tree under tools. This contains some useful stuff, to
make that available, import the necessary Make helper file and then add
it to the -I flags.

There still aren't that many headers so also just smush all of them into
SHARED_DEPS instead of starting to do any header dependency hocus pocus.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/shared/shared.mk | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/shared/shared.mk b/tools/testing/shared/shared.mk
index 923ee2492256b693c5cf16cc014d9d2410be5457..937aaa7623320da1085a8e0f43f6a728ddd3ab1c 100644
--- a/tools/testing/shared/shared.mk
+++ b/tools/testing/shared/shared.mk
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
+include ../../scripts/Makefile.arch
 
-CFLAGS += -I../shared -I. -I../../include -I../../../lib -g -Og -Wall \
+CFLAGS += -I../shared -I. -I../../include -I../../arch/$(SRCARCH)/include \
+	  -I../../../lib -g -Og -Wall \
 	  -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined
 LDFLAGS += -fsanitize=address -fsanitize=undefined
 LDLIBS += -lpthread -lurcu
@@ -11,6 +13,7 @@ SHARED_DEPS = Makefile ../shared/shared.mk ../shared/*.h generated/map-shift.h \
 	generated/bit-length.h generated/autoconf.h \
 	../../include/linux/*.h \
 	../../include/asm/*.h \
+	../../arch/$(SRCARCH)/include/asm/*.h \
 	../../../include/linux/xarray.h \
 	../../../include/linux/maple_tree.h \
 	../../../include/linux/radix-tree.h \

-- 
2.50.1

Re: [PATCH 1/3] tools: testing: Allow importing arch headers in shared.mk

[Pedro Falcato]

On Wed, Aug 27, 2025 at 11:04:41AM +0000, Brendan Jackman wrote:
> There is an arch/ tree under tools. This contains some useful stuff, to
> make that available, import the necessary Make helper file and then add
> it to the -I flags.
> 
> There still aren't that many headers so also just smush all of them into
> SHARED_DEPS instead of starting to do any header dependency hocus pocus.
>

I was a little confused as to why this patchset was safe, and - yeah - i missed
the arch/ under tools/.

There are asm-generic headers so hopefully those fully take care of !x86? Did
you check?

In any case:
Acked-by: Pedro Falcato <pfalcato@suse.de>

-- 
Pedro

Re: [PATCH 1/3] tools: testing: Allow importing arch headers in shared.mk

[Brendan Jackman]

On Wed Aug 27, 2025 at 12:50 PM UTC, Pedro Falcato wrote:
> On Wed, Aug 27, 2025 at 11:04:41AM +0000, Brendan Jackman wrote:
>> There is an arch/ tree under tools. This contains some useful stuff, to
>> make that available, import the necessary Make helper file and then add
>> it to the -I flags.
>> 
>> There still aren't that many headers so also just smush all of them into
>> SHARED_DEPS instead of starting to do any header dependency hocus pocus.
>>
>
> I was a little confused as to why this patchset was safe, and - yeah - i missed
> the arch/ under tools/.
>
> There are asm-generic headers so hopefully those fully take care of !x86? 

[Confidently but wihout evidence] yep, without a doubt!

> Did you check?

Um, OK that's fair question. This doesn't support cross-compilation so
you actually need a non-x86 environment.

<begin verbose notes, tl;dr: yeah seems to work for arm64>

I have Nix set up to build aarch64 binaries via binfmt_misc though so I
tried using that...

❯❯  cat /etc/nix/nix.conf
# see https://nixos.org/manual/nix/stable/command-ref/conf-file

sandbox = true

max-jobs = 96

extra-platforms = aarch64-linux

So I dropped this into the root of the kernel repo (and `git add`ed it
otherwise Nix pretends it's not there):

❯❯  cat flake.nix 
{
  inputs = {
    nixpkgs.url = "github:nixos/nixpkgs?ref=nixos-25.05";
  };

  outputs =
    {
      self,
      nixpkgs,
    }:
    let
      system = "aarch64-linux";
      pkgs = import nixpkgs { inherit system; };
    in
    {
      formatter."${system}" = pkgs.nixfmt-tree;

      packages."${system}".vma-tests = pkgs.stdenv.mkDerivation {
        name = "vma-tests";
        src = ./.;
        # (Pretty sure this is not the correct way to do this, there must be
        # some mkDerivation arg that's equivalent to make's -C flag)
        buildPhase = "make -C tools/testing/vma";
        nativeBuildInputs = [ pkgs.liburcu ];

        installPhase = ''
          mkdir $out
          cp tools/testing/vma/vma $out
        '';

	checkPhase = ''./tools/testing/vma'';
      };
    };
}

And, yeah at least it compiles, and that checkPhase should have run the
VMA tests:

❯❯  nix build .#packages.aarch64-linux.vma-tests
warning: Git tree '/usr/local/google/home/jackmanb/src/linux/linux' is dirty

❯❯  file ./result/vma 
./result/vma: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), dynamically linked, interpreter /nix/store/r0pjdp81mmd7dvk5pv1ch75hrbbw60xb-glibc-2.40-66/lib/ld-linux-aarch64.so.1, for GNU/Linux 3.10.0, with debug_info, not stripped

So... that was easier than expected :)

Re: [PATCH 1/3] tools: testing: Allow importing arch headers in shared.mk

[Liam R. Howlett]

* Brendan Jackman <jackmanb@google.com> [250827 11:07]:
> On Wed Aug 27, 2025 at 12:50 PM UTC, Pedro Falcato wrote:
> > On Wed, Aug 27, 2025 at 11:04:41AM +0000, Brendan Jackman wrote:
> >> There is an arch/ tree under tools. This contains some useful stuff, to
> >> make that available, import the necessary Make helper file and then add
> >> it to the -I flags.
> >> 
> >> There still aren't that many headers so also just smush all of them into
> >> SHARED_DEPS instead of starting to do any header dependency hocus pocus.
> >>
> >
> > I was a little confused as to why this patchset was safe, and - yeah - i missed
> > the arch/ under tools/.
> >
> > There are asm-generic headers so hopefully those fully take care of !x86? 
> 
> [Confidently but wihout evidence] yep, without a doubt!
> 
> > Did you check?
> 
> Um, OK that's fair question. This doesn't support cross-compilation so
> you actually need a non-x86 environment.

btw, BUILD=32 works in the radix tree directory for 32bit testing.
Although there appears to be some warnings right now for it.

Thanks,
Liam

Re: [PATCH 1/3] tools: testing: Allow importing arch headers in shared.mk

[Liam R. Howlett]

* Brendan Jackman <jackmanb@google.com> [250827 07:04]:
> There is an arch/ tree under tools. This contains some useful stuff, to
> make that available, import the necessary Make helper file and then add
> it to the -I flags.
> 
> There still aren't that many headers so also just smush all of them into
> SHARED_DEPS instead of starting to do any header dependency hocus pocus.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Thanks Brendan

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  tools/testing/shared/shared.mk | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/shared/shared.mk b/tools/testing/shared/shared.mk
> index 923ee2492256b693c5cf16cc014d9d2410be5457..937aaa7623320da1085a8e0f43f6a728ddd3ab1c 100644
> --- a/tools/testing/shared/shared.mk
> +++ b/tools/testing/shared/shared.mk
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
> +include ../../scripts/Makefile.arch
>  
> -CFLAGS += -I../shared -I. -I../../include -I../../../lib -g -Og -Wall \
> +CFLAGS += -I../shared -I. -I../../include -I../../arch/$(SRCARCH)/include \
> +	  -I../../../lib -g -Og -Wall \
>  	  -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined
>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>  LDLIBS += -lpthread -lurcu
> @@ -11,6 +13,7 @@ SHARED_DEPS = Makefile ../shared/shared.mk ../shared/*.h generated/map-shift.h \
>  	generated/bit-length.h generated/autoconf.h \
>  	../../include/linux/*.h \
>  	../../include/asm/*.h \
> +	../../arch/$(SRCARCH)/include/asm/*.h \
>  	../../../include/linux/xarray.h \
>  	../../../include/linux/maple_tree.h \
>  	../../../include/linux/radix-tree.h \
> 
> -- 
> 2.50.1
> 

Re: [PATCH 1/3] tools: testing: Allow importing arch headers in shared.mk

[Lorenzo Stoakes]

On Wed, Aug 27, 2025 at 11:04:41AM +0000, Brendan Jackman wrote:
> There is an arch/ tree under tools. This contains some useful stuff, to
> make that available, import the necessary Make helper file and then add
> it to the -I flags.
>
> There still aren't that many headers so also just smush all of them into
> SHARED_DEPS instead of starting to do any header dependency hocus pocus.
>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

LGTM, + works locally so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  tools/testing/shared/shared.mk | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/shared/shared.mk b/tools/testing/shared/shared.mk
> index 923ee2492256b693c5cf16cc014d9d2410be5457..937aaa7623320da1085a8e0f43f6a728ddd3ab1c 100644
> --- a/tools/testing/shared/shared.mk
> +++ b/tools/testing/shared/shared.mk
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
> +include ../../scripts/Makefile.arch
>
> -CFLAGS += -I../shared -I. -I../../include -I../../../lib -g -Og -Wall \
> +CFLAGS += -I../shared -I. -I../../include -I../../arch/$(SRCARCH)/include \
> +	  -I../../../lib -g -Og -Wall \

We're also adding lib! Very very nitty but maybe mention in commit msg. But not
a big deal obviously!

>  	  -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined
>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>  LDLIBS += -lpthread -lurcu
> @@ -11,6 +13,7 @@ SHARED_DEPS = Makefile ../shared/shared.mk ../shared/*.h generated/map-shift.h \
>  	generated/bit-length.h generated/autoconf.h \
>  	../../include/linux/*.h \
>  	../../include/asm/*.h \
> +	../../arch/$(SRCARCH)/include/asm/*.h \

I'm no Makefile master, but I'm guessing this will always be defined _somehow_ :)?

>  	../../../include/linux/xarray.h \
>  	../../../include/linux/maple_tree.h \
>  	../../../include/linux/radix-tree.h \
>
> --
> 2.50.1
>

Re: [PATCH 1/3] tools: testing: Allow importing arch headers in shared.mk

[Brendan Jackman]

On Thu Aug 28, 2025 at 10:23 AM UTC, Lorenzo Stoakes wrote:
> On Wed, Aug 27, 2025 at 11:04:41AM +0000, Brendan Jackman wrote:
>> There is an arch/ tree under tools. This contains some useful stuff, to
>> make that available, import the necessary Make helper file and then add
>> it to the -I flags.
>>
>> There still aren't that many headers so also just smush all of them into
>> SHARED_DEPS instead of starting to do any header dependency hocus pocus.
>>
>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>
> LGTM, + works locally so:
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
>> ---
>>  tools/testing/shared/shared.mk | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/shared/shared.mk b/tools/testing/shared/shared.mk
>> index 923ee2492256b693c5cf16cc014d9d2410be5457..937aaa7623320da1085a8e0f43f6a728ddd3ab1c 100644
>> --- a/tools/testing/shared/shared.mk
>> +++ b/tools/testing/shared/shared.mk
>> @@ -1,6 +1,8 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> +include ../../scripts/Makefile.arch
>>
>> -CFLAGS += -I../shared -I. -I../../include -I../../../lib -g -Og -Wall \
>> +CFLAGS += -I../shared -I. -I../../include -I../../arch/$(SRCARCH)/include \
>> +	  -I../../../lib -g -Og -Wall \
>
> We're also adding lib! Very very nitty but maybe mention in commit msg. But not
> a big deal obviously!

No that's already there, it just looks new coz I split up the line!

>>  	  -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined
>>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>>  LDLIBS += -lpthread -lurcu
>> @@ -11,6 +13,7 @@ SHARED_DEPS = Makefile ../shared/shared.mk ../shared/*.h generated/map-shift.h \
>>  	generated/bit-length.h generated/autoconf.h \
>>  	../../include/linux/*.h \
>>  	../../include/asm/*.h \
>> +	../../arch/$(SRCARCH)/include/asm/*.h \
>
> I'm no Makefile master, but I'm guessing this will always be defined _somehow_ :)?

Ah yeah this is what the Makefile.arch include is for - I'll update the
commit message to be explicit about that. v2 incoming shortly...

>>  	../../../include/linux/xarray.h \
>>  	../../../include/linux/maple_tree.h \
>>  	../../../include/linux/radix-tree.h \
>>
>> --
>> 2.50.1
>>

[PATCH 2/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Brendan Jackman]

The shared userspace logic used for unit-testing radix-tree and VMA code
currently has its own replacements for atomics helpers. This is not
needed as the necessary APIs already have userspace implementations in
the tools tree. Switching over to that allows deleting a bit of code.

Note that the implementation is different; while the version being
deleted here is implemented using liburcu, the existing version in tools
uses either x86 asm or compiler builtins. It's assumed that both are
equally likely to be correct.

The tools tree's version of atomic_t is a struct type while the version
being deleted was just a typedef of an integer. This means it's no
longer valid to call __sync_bool_compare_and_swap() directly on it. One
option would be to just peek into the struct and call it on the field,
but it seems a little cleaner to just use the corresponding atomic.h
API. On non-x86 archs this is implemented using
__sync_val_compare_and_swap(). It's not clear why the old version uses
the bool variant instead of the generic "val" one, for now it's assumed
that this was a mistake.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/shared/linux/maple_tree.h |  6 ++----
 tools/testing/vma/linux/atomic.h        | 17 -----------------
 tools/testing/vma/vma_internal.h        |  3 ++-
 3 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/tools/testing/shared/linux/maple_tree.h b/tools/testing/shared/linux/maple_tree.h
index f67d47d32857cee296c2784da57825c9a31cd340..7d0fadef0f11624dbb110ad351aabdc79a19dcd2 100644
--- a/tools/testing/shared/linux/maple_tree.h
+++ b/tools/testing/shared/linux/maple_tree.h
@@ -1,7 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0+ */
-#define atomic_t int32_t
-#define atomic_inc(x) uatomic_inc(x)
-#define atomic_read(x) uatomic_read(x)
-#define atomic_set(x, y) uatomic_set(x, y)
+#include <linux/atomic.h>
+
 #define U8_MAX UCHAR_MAX
 #include "../../../../include/linux/maple_tree.h"
diff --git a/tools/testing/vma/linux/atomic.h b/tools/testing/vma/linux/atomic.h
deleted file mode 100644
index 788c597c4fdea7392307de93ff4459453b96179b..0000000000000000000000000000000000000000
--- a/tools/testing/vma/linux/atomic.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/* 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) uatomic_set(x, y)
-#define U8_MAX UCHAR_MAX
-
-#ifndef atomic_cmpxchg_relaxed
-#define  atomic_cmpxchg_relaxed		uatomic_cmpxchg
-#define  atomic_cmpxchg_release         uatomic_cmpxchg
-#endif /* atomic_cmpxchg_relaxed */
-
-#endif	/* _LINUX_ATOMIC_H */
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 3639aa8dd2b06ebe5b9cfcfe6669994fd38c482d..a720a4e6bada83e6b32e76762089eeec35ba8fac 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -21,6 +21,7 @@
 
 #include <stdlib.h>
 
+#include <linux/atomic.h>
 #include <linux/list.h>
 #include <linux/maple_tree.h>
 #include <linux/mm.h>
@@ -1381,7 +1382,7 @@ static inline int mapping_map_writable(struct address_space *mapping)
 	do {
 		if (c < 0)
 			return -EPERM;
-	} while (!__sync_bool_compare_and_swap(&mapping->i_mmap_writable, c, c+1));
+	} while (!atomic_cmpxchg(&mapping->i_mmap_writable, c, c+1));
 
 	return 0;
 }

-- 
2.50.1

Re: [PATCH 2/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Pedro Falcato]

On Wed, Aug 27, 2025 at 11:04:42AM +0000, Brendan Jackman wrote:
> The shared userspace logic used for unit-testing radix-tree and VMA code
> currently has its own replacements for atomics helpers. This is not
> needed as the necessary APIs already have userspace implementations in
> the tools tree. Switching over to that allows deleting a bit of code.
> 
> Note that the implementation is different; while the version being
> deleted here is implemented using liburcu, the existing version in tools
> uses either x86 asm or compiler builtins. It's assumed that both are
> equally likely to be correct.
> 
> The tools tree's version of atomic_t is a struct type while the version
> being deleted was just a typedef of an integer. This means it's no
> longer valid to call __sync_bool_compare_and_swap() directly on it. One
> option would be to just peek into the struct and call it on the field,
> but it seems a little cleaner to just use the corresponding atomic.h
> API. On non-x86 archs this is implemented using
> __sync_val_compare_and_swap(). It's not clear why the old version uses
> the bool variant instead of the generic "val" one, for now it's assumed
> that this was a mistake.
>

I don't think it's a mistake. Namely we're checking if the cmpxchg occured.
So in the new version you'll have trouble incrementing i_mmap_writeable from
0 to 1, where in practice you should (AIUI) see 0 -> 1 (old val = 0, retry) -> 2,
which is obviously not correct here.

At the very least you'll need some:

do {
} while(atomic_cmpxchg(&mapping->i_mmap_writeable, c, c+1) != c);

to keep the same semantics.

-- 
Pedro

Re: [PATCH 2/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Brendan Jackman]

On Wed Aug 27, 2025 at 12:56 PM UTC, Pedro Falcato wrote:
> On Wed, Aug 27, 2025 at 11:04:42AM +0000, Brendan Jackman wrote:
>> The shared userspace logic used for unit-testing radix-tree and VMA code
>> currently has its own replacements for atomics helpers. This is not
>> needed as the necessary APIs already have userspace implementations in
>> the tools tree. Switching over to that allows deleting a bit of code.
>> 
>> Note that the implementation is different; while the version being
>> deleted here is implemented using liburcu, the existing version in tools
>> uses either x86 asm or compiler builtins. It's assumed that both are
>> equally likely to be correct.
>> 
>> The tools tree's version of atomic_t is a struct type while the version
>> being deleted was just a typedef of an integer. This means it's no
>> longer valid to call __sync_bool_compare_and_swap() directly on it. One
>> option would be to just peek into the struct and call it on the field,
>> but it seems a little cleaner to just use the corresponding atomic.h
>> API. On non-x86 archs this is implemented using
>> __sync_val_compare_and_swap(). It's not clear why the old version uses
>> the bool variant instead of the generic "val" one, for now it's assumed
>> that this was a mistake.
>>
>
> I don't think it's a mistake. Namely we're checking if the cmpxchg occured.
> So in the new version you'll have trouble incrementing i_mmap_writeable from
> 0 to 1, where in practice you should (AIUI) see 0 -> 1 (old val = 0, retry) -> 2,
> which is obviously not correct here.
>
> At the very least you'll need some:
>
> do {
> } while(atomic_cmpxchg(&mapping->i_mmap_writeable, c, c+1) != c);

Oops, yeah my code is total nonsense here - thanks for paying attention.
I guess I "got away with it" because there's probably no actual races
going on when I run the tests...

Anyway I'll apply my brain properly next time I get the chance and send
a v2.

Thanks for the review!

Re: [PATCH 2/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Liam R. Howlett]

* Brendan Jackman <jackmanb@google.com> [250827 07:04]:
> The shared userspace logic used for unit-testing radix-tree and VMA code
> currently has its own replacements for atomics helpers. This is not
> needed as the necessary APIs already have userspace implementations in
> the tools tree. Switching over to that allows deleting a bit of code.
> 
> Note that the implementation is different; while the version being
> deleted here is implemented using liburcu, the existing version in tools
> uses either x86 asm or compiler builtins. It's assumed that both are
> equally likely to be correct.
> 
> The tools tree's version of atomic_t is a struct type while the version
> being deleted was just a typedef of an integer. This means it's no
> longer valid to call __sync_bool_compare_and_swap() directly on it. One
> option would be to just peek into the struct and call it on the field,
> but it seems a little cleaner to just use the corresponding atomic.h
> API. On non-x86 archs this is implemented using
> __sync_val_compare_and_swap(). It's not clear why the old version uses
> the bool variant instead of the generic "val" one, for now it's assumed
> that this was a mistake.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>  tools/testing/shared/linux/maple_tree.h |  6 ++----
                              ^^^^^
Did you say radix-tree?

I was going to accept this because I put my code in the same directory,
but since you'll be respinning..

>  tools/testing/vma/linux/atomic.h        | 17 -----------------
>  tools/testing/vma/vma_internal.h        |  3 ++-
>  3 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/testing/shared/linux/maple_tree.h b/tools/testing/shared/linux/maple_tree.h
> index f67d47d32857cee296c2784da57825c9a31cd340..7d0fadef0f11624dbb110ad351aabdc79a19dcd2 100644
> --- a/tools/testing/shared/linux/maple_tree.h
> +++ b/tools/testing/shared/linux/maple_tree.h
> @@ -1,7 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0+ */
> -#define atomic_t int32_t
> -#define atomic_inc(x) uatomic_inc(x)
> -#define atomic_read(x) uatomic_read(x)
> -#define atomic_set(x, y) uatomic_set(x, y)
> +#include <linux/atomic.h>
> +
>  #define U8_MAX UCHAR_MAX
>  #include "../../../../include/linux/maple_tree.h"
> diff --git a/tools/testing/vma/linux/atomic.h b/tools/testing/vma/linux/atomic.h
> deleted file mode 100644
> index 788c597c4fdea7392307de93ff4459453b96179b..0000000000000000000000000000000000000000
> --- a/tools/testing/vma/linux/atomic.h
> +++ /dev/null
> @@ -1,17 +0,0 @@
> -/* 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) uatomic_set(x, y)
> -#define U8_MAX UCHAR_MAX
> -
> -#ifndef atomic_cmpxchg_relaxed
> -#define  atomic_cmpxchg_relaxed		uatomic_cmpxchg
> -#define  atomic_cmpxchg_release         uatomic_cmpxchg
> -#endif /* atomic_cmpxchg_relaxed */
> -
> -#endif	/* _LINUX_ATOMIC_H */
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 3639aa8dd2b06ebe5b9cfcfe6669994fd38c482d..a720a4e6bada83e6b32e76762089eeec35ba8fac 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -21,6 +21,7 @@
>  
>  #include <stdlib.h>
>  
> +#include <linux/atomic.h>
>  #include <linux/list.h>
>  #include <linux/maple_tree.h>
>  #include <linux/mm.h>
> @@ -1381,7 +1382,7 @@ static inline int mapping_map_writable(struct address_space *mapping)
>  	do {
>  		if (c < 0)
>  			return -EPERM;
> -	} while (!__sync_bool_compare_and_swap(&mapping->i_mmap_writable, c, c+1));
> +	} while (!atomic_cmpxchg(&mapping->i_mmap_writable, c, c+1));
>  
>  	return 0;
>  }
> 
> -- 
> 2.50.1
> 

Re: [PATCH 2/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Brendan Jackman]

On Thu Aug 28, 2025 at 1:05 AM UTC, Liam R. Howlett wrote:
> * Brendan Jackman <jackmanb@google.com> [250827 07:04]:
>> The shared userspace logic used for unit-testing radix-tree and VMA code
>> currently has its own replacements for atomics helpers. This is not
>> needed as the necessary APIs already have userspace implementations in
>> the tools tree. Switching over to that allows deleting a bit of code.
>> 
>> Note that the implementation is different; while the version being
>> deleted here is implemented using liburcu, the existing version in tools
>> uses either x86 asm or compiler builtins. It's assumed that both are
>> equally likely to be correct.
>> 
>> The tools tree's version of atomic_t is a struct type while the version
>> being deleted was just a typedef of an integer. This means it's no
>> longer valid to call __sync_bool_compare_and_swap() directly on it. One
>> option would be to just peek into the struct and call it on the field,
>> but it seems a little cleaner to just use the corresponding atomic.h
>> API. On non-x86 archs this is implemented using
>> __sync_val_compare_and_swap(). It's not clear why the old version uses
>> the bool variant instead of the generic "val" one, for now it's assumed
>> that this was a mistake.
>> 
>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>> ---
>>  tools/testing/shared/linux/maple_tree.h |  6 ++----
>                               ^^^^^
> Did you say radix-tree?
>
> I was going to accept this because I put my code in the same directory,
> but since you'll be respinning..

Yeah, it's only the maple tree tests specifically that are affected. Did
I understand correctly that you're asking me to reword the commit
messages to avoid confusion? If so, yep good idea will do.

Re: [PATCH 2/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Lorenzo Stoakes]

On Wed, Aug 27, 2025 at 11:04:42AM +0000, Brendan Jackman wrote:
> The shared userspace logic used for unit-testing radix-tree and VMA code
> currently has its own replacements for atomics helpers. This is not
> needed as the necessary APIs already have userspace implementations in
> the tools tree. Switching over to that allows deleting a bit of code.
>
> Note that the implementation is different; while the version being
> deleted here is implemented using liburcu, the existing version in tools
> uses either x86 asm or compiler builtins. It's assumed that both are
> equally likely to be correct.
>
> The tools tree's version of atomic_t is a struct type while the version
> being deleted was just a typedef of an integer. This means it's no
> longer valid to call __sync_bool_compare_and_swap() directly on it. One
> option would be to just peek into the struct and call it on the field,
> but it seems a little cleaner to just use the corresponding atomic.h
> API. On non-x86 archs this is implemented using
> __sync_val_compare_and_swap(). It's not clear why the old version uses
> the bool variant instead of the generic "val" one, for now it's assumed
> that this was a mistake.
>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

OK so on basis you fix Liam + Pedro's comments, feel free to add:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

On respin.

Thanks!

> ---
>  tools/testing/shared/linux/maple_tree.h |  6 ++----
>  tools/testing/vma/linux/atomic.h        | 17 -----------------

Thanks :)

>  tools/testing/vma/vma_internal.h        |  3 ++-
>  3 files changed, 4 insertions(+), 22 deletions(-)
>
> diff --git a/tools/testing/shared/linux/maple_tree.h b/tools/testing/shared/linux/maple_tree.h
> index f67d47d32857cee296c2784da57825c9a31cd340..7d0fadef0f11624dbb110ad351aabdc79a19dcd2 100644
> --- a/tools/testing/shared/linux/maple_tree.h
> +++ b/tools/testing/shared/linux/maple_tree.h
> @@ -1,7 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0+ */
> -#define atomic_t int32_t
> -#define atomic_inc(x) uatomic_inc(x)
> -#define atomic_read(x) uatomic_read(x)
> -#define atomic_set(x, y) uatomic_set(x, y)

This is nice!

> +#include <linux/atomic.h>
> +
>  #define U8_MAX UCHAR_MAX
>  #include "../../../../include/linux/maple_tree.h"
> diff --git a/tools/testing/vma/linux/atomic.h b/tools/testing/vma/linux/atomic.h
> deleted file mode 100644
> index 788c597c4fdea7392307de93ff4459453b96179b..0000000000000000000000000000000000000000
> --- a/tools/testing/vma/linux/atomic.h
> +++ /dev/null
> @@ -1,17 +0,0 @@
> -/* 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) uatomic_set(x, y)
> -#define U8_MAX UCHAR_MAX
> -
> -#ifndef atomic_cmpxchg_relaxed
> -#define  atomic_cmpxchg_relaxed		uatomic_cmpxchg
> -#define  atomic_cmpxchg_release         uatomic_cmpxchg
> -#endif /* atomic_cmpxchg_relaxed */
> -
> -#endif	/* _LINUX_ATOMIC_H */

This is also nice!

> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 3639aa8dd2b06ebe5b9cfcfe6669994fd38c482d..a720a4e6bada83e6b32e76762089eeec35ba8fac 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -21,6 +21,7 @@
>
>  #include <stdlib.h>
>
> +#include <linux/atomic.h>
>  #include <linux/list.h>
>  #include <linux/maple_tree.h>
>  #include <linux/mm.h>
> @@ -1381,7 +1382,7 @@ static inline int mapping_map_writable(struct address_space *mapping)
>  	do {
>  		if (c < 0)
>  			return -EPERM;
> -	} while (!__sync_bool_compare_and_swap(&mapping->i_mmap_writable, c, c+1));
> +	} while (!atomic_cmpxchg(&mapping->i_mmap_writable, c, c+1));

Obv. ref. Pedro's reply.

>
>  	return 0;
>  }
>
> --
> 2.50.1
>

[PATCH 3/3] tools: testing: Support EXTRA_CFLAGS in shared.mk

[Brendan Jackman]

This allows the user to set cflags when building tests that use this
shared build infrastructure.

For example, it enables building with -Werror so that patch-check
scripts will fail:

	make -C tools/testing/vma -j EXTRA_CFLAGS=-Werror

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/shared/shared.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/shared/shared.mk b/tools/testing/shared/shared.mk
index 937aaa7623320da1085a8e0f43f6a728ddd3ab1c..5bcdf26c8a9d51ab2cbd264f2f8a7445d7c036e3 100644
--- a/tools/testing/shared/shared.mk
+++ b/tools/testing/shared/shared.mk
@@ -4,6 +4,7 @@ include ../../scripts/Makefile.arch
 CFLAGS += -I../shared -I. -I../../include -I../../arch/$(SRCARCH)/include \
 	  -I../../../lib -g -Og -Wall \
 	  -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined
+CFLAGS += $(EXTRA_CFLAGS)
 LDFLAGS += -fsanitize=address -fsanitize=undefined
 LDLIBS += -lpthread -lurcu
 LIBS := slab.o find_bit.o bitmap.o hweight.o vsprintf.o

-- 
2.50.1

Re: [PATCH 3/3] tools: testing: Support EXTRA_CFLAGS in shared.mk

[Pedro Falcato]

On Wed, Aug 27, 2025 at 11:04:43AM +0000, Brendan Jackman wrote:
> This allows the user to set cflags when building tests that use this
> shared build infrastructure.
> 
> For example, it enables building with -Werror so that patch-check
> scripts will fail:
> 
> 	make -C tools/testing/vma -j EXTRA_CFLAGS=-Werror
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Acked-by: Pedro Falcato <pfalcato@suse.de>

-- 
Pedro

Re: [PATCH 3/3] tools: testing: Support EXTRA_CFLAGS in shared.mk

[Liam R. Howlett]

* Brendan Jackman <jackmanb@google.com> [250827 07:04]:
> This allows the user to set cflags when building tests that use this
> shared build infrastructure.
> 
> For example, it enables building with -Werror so that patch-check
> scripts will fail:
> 
> 	make -C tools/testing/vma -j EXTRA_CFLAGS=-Werror
> 

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>  tools/testing/shared/shared.mk | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/shared/shared.mk b/tools/testing/shared/shared.mk
> index 937aaa7623320da1085a8e0f43f6a728ddd3ab1c..5bcdf26c8a9d51ab2cbd264f2f8a7445d7c036e3 100644
> --- a/tools/testing/shared/shared.mk
> +++ b/tools/testing/shared/shared.mk
> @@ -4,6 +4,7 @@ include ../../scripts/Makefile.arch
>  CFLAGS += -I../shared -I. -I../../include -I../../arch/$(SRCARCH)/include \
>  	  -I../../../lib -g -Og -Wall \
>  	  -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined
> +CFLAGS += $(EXTRA_CFLAGS)
>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>  LDLIBS += -lpthread -lurcu
>  LIBS := slab.o find_bit.o bitmap.o hweight.o vsprintf.o
> 
> -- 
> 2.50.1
> 

Re: [PATCH 3/3] tools: testing: Support EXTRA_CFLAGS in shared.mk

[Lorenzo Stoakes]

On Wed, Aug 27, 2025 at 11:04:43AM +0000, Brendan Jackman wrote:
> This allows the user to set cflags when building tests that use this
> shared build infrastructure.
>
> For example, it enables building with -Werror so that patch-check
> scripts will fail:
>
> 	make -C tools/testing/vma -j EXTRA_CFLAGS=-Werror
>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Nice this is useful thanks!

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  tools/testing/shared/shared.mk | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/shared/shared.mk b/tools/testing/shared/shared.mk
> index 937aaa7623320da1085a8e0f43f6a728ddd3ab1c..5bcdf26c8a9d51ab2cbd264f2f8a7445d7c036e3 100644
> --- a/tools/testing/shared/shared.mk
> +++ b/tools/testing/shared/shared.mk
> @@ -4,6 +4,7 @@ include ../../scripts/Makefile.arch
>  CFLAGS += -I../shared -I. -I../../include -I../../arch/$(SRCARCH)/include \
>  	  -I../../../lib -g -Og -Wall \
>  	  -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined
> +CFLAGS += $(EXTRA_CFLAGS)
>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>  LDLIBS += -lpthread -lurcu
>  LIBS := slab.o find_bit.o bitmap.o hweight.o vsprintf.o
>
> --
> 2.50.1
>

Re: [PATCH 0/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Lorenzo Stoakes]

On Wed, Aug 27, 2025 at 11:04:40AM +0000, Brendan Jackman wrote:
> De-duplicating this lets us delete a bit of code.

Thanks very much! This is nice stuff.

>
> Ulterior motive: I'm working on a new set of the userspace-based unit
> tests, which will need the atomics API too. That would involve even more
> duplication, so while the win in this patchset alone is very minimal, it
> looks a lot more significant with my other WIP patchset.

I hope that my + Liam's work helped inspire you :)

>
> I've tested these commands:
>
> make -C tools/testing/vma -j
> tools/testing/vma/vma
>
> make -C tools/testing/radix-tree -j
> tools/testing/radix-tree/main
>
> Note the EXTRA_CFLAGS patch is actually orthogonal, let me know if you'd
> prefer I send it separately.

It's fine!

Cheers, Lorenzo

Re: [PATCH 0/3] tools: testing: Use existing atomic.h for vma/radix-tree tests

[Brendan Jackman]

On Thu Aug 28, 2025 at 10:28 AM UTC, Lorenzo Stoakes wrote:
> On Wed, Aug 27, 2025 at 11:04:40AM +0000, Brendan Jackman wrote:
>> De-duplicating this lets us delete a bit of code.
>
> Thanks very much! This is nice stuff.
>
>>
>> Ulterior motive: I'm working on a new set of the userspace-based unit
>> tests, which will need the atomics API too. That would involve even more
>> duplication, so while the win in this patchset alone is very minimal, it
>> looks a lot more significant with my other WIP patchset.
>
> I hope that my + Liam's work helped inspire you :)

Certainly! Unfortunately after spending most of the last week on it I
decided I need to cut my losses for the time being. I can see where it
needs to get to but it's hard to say how many more days I'll have to
pour into fiddling around splitting headers and moving definitions etc
etc etc. This unknown number of days is hard to justify investing right
now, given that I think there's a high risk people see the amount of
code movement and go "please immediately step away from my codebase":

❯❯  git diff --stat origin/master..buddy-tests
 include/linux/buddy.h                    |   58 ++
 include/linux/gfp.h                      |   36 +-
 include/linux/mm.h                       |   13 -
 include/linux/mm_types.h                 |  103 ++-
 include/linux/mmzone.h                   |  942 +-------------------
 include/linux/mmzone_types.h             | 1080 ++++++++++++++++++++++
 include/linux/nodemask_types.h           |    2 +-
 include/linux/page-flags.h               |   52 +-
 include/linux/page-isolation.h           |    8 -
 include/trace/events/kmem.h              |    1 +
 kernel/bounds.c                          |    9 +-
 mm/Makefile                              |    2 +-
 mm/buddy.c                               | 3618 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mm/buddy.h                               |  738 +++++++++++++++
 mm/buddy_internal.h                      |  136 +++
 mm/compaction.c                          |    1 +
 mm/internal.h                            |  300 +++----
 mm/page_alloc.c                          | 4403 +++---------------------------------------------------------------------------------------
 mm/page_isolation.c                      |    1 +
 mm/page_reporting.c                      |    1 +
 mm/show_mem.c                            |    1 +
 mm/vmstat.c                              |    1 +
 tools/include/linux/atomic.h             |   19 +
 tools/include/linux/bitops.h             |    9 +
 tools/include/linux/cache.h              |    2 +
 tools/include/linux/llist.h              |  317 +++++++
 tools/include/linux/math.h               |   12 +
 tools/include/linux/spinlock.h           |    1 +
 tools/testing/buddy/.gitignore           |    7 +
 tools/testing/buddy/Makefile             |   18 +
 tools/testing/buddy/buddy.c              |   40 +
 tools/testing/buddy/buddy_internal.h     |  256 ++++++
 tools/testing/buddy/linux/mm_types.h     |   54 ++
 tools/testing/buddy/linux/mmdebug.h      |   13 +
 tools/testing/buddy/linux/nr_pageflags.h |   12 +
 tools/testing/buddy/linux/seqlock.h      |    5 +
 tools/testing/shared/linux/lockdep.h     |    3 +
 tools/testing/shared/linux/maple_tree.h  |    6 +-
 tools/testing/shared/shared.mk           |    6 +-
 tools/testing/vma/linux/atomic.h         |   17 -
 tools/testing/vma/vma_internal.h         |    3 +-
 41 files changed, 6776 insertions(+), 5530 deletions(-)

Anyway, at least it resulted in a nice little cleanup for the existing
tests. And maybe I'll get back to it soon when I'm back to being
frustrated about how hard it is to be sure my page_alloc.c code works.