[v1] radix tree test suite: put definition of bitmap_clear() into lib/bitmap.c

[PATCH] radix tree test suite: put definition of bitmap_clear() into lib/bitmap.c

Posted by Wei Yang 1 year, 5 months ago

Instead of keeping a bitmap.c in radix-tree own directory, we can use
the common implementation in lib/ directory.

Just move the definition and link some related libs, no functional
change.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Matthew Wilcox <willy@infradead.org>
---
 tools/lib/bitmap.c                | 20 ++++++++++++++++++++
 tools/testing/radix-tree/Makefile |  4 ++--
 tools/testing/radix-tree/bitmap.c | 23 -----------------------
 3 files changed, 22 insertions(+), 25 deletions(-)
 delete mode 100644 tools/testing/radix-tree/bitmap.c

diff --git a/tools/lib/bitmap.c b/tools/lib/bitmap.c
index c3e4871967bc..a96dbf001244 100644
--- a/tools/lib/bitmap.c
+++ b/tools/lib/bitmap.c
@@ -100,3 +100,23 @@ bool __bitmap_intersects(const unsigned long *bitmap1,
 			return true;
 	return false;
 }
+
+void bitmap_clear(unsigned long *map, unsigned int start, int len)
+{
+	unsigned long *p = map + BIT_WORD(start);
+	const unsigned int size = start + len;
+	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
+	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
+
+	while (len - bits_to_clear >= 0) {
+		*p &= ~mask_to_clear;
+		len -= bits_to_clear;
+		bits_to_clear = BITS_PER_LONG;
+		mask_to_clear = ~0UL;
+		p++;
+	}
+	if (len) {
+		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
+		*p &= ~mask_to_clear;
+	}
+}
diff --git a/tools/testing/radix-tree/Makefile b/tools/testing/radix-tree/Makefile
index 7527f738b4a1..505cc24a8b9c 100644
--- a/tools/testing/radix-tree/Makefile
+++ b/tools/testing/radix-tree/Makefile
@@ -5,8 +5,8 @@ CFLAGS += -I. -I../../include -I../../../lib -g -Og -Wall \
 LDFLAGS += -fsanitize=address -fsanitize=undefined
 LDLIBS+= -lpthread -lurcu
 TARGETS = main idr-test multiorder xarray maple
-CORE_OFILES := xarray.o radix-tree.o idr.o linux.o test.o find_bit.o bitmap.o \
-			 slab.o maple.o
+LIBS := find_bit.o bitmap.o hweight.o vsprintf.o
+CORE_OFILES := xarray.o radix-tree.o idr.o linux.o test.o slab.o maple.o $(LIBS)
 OFILES = main.o $(CORE_OFILES) regression1.o regression2.o regression3.o \
 	 regression4.o tag_check.o multiorder.o idr-test.o iteration_check.o \
 	 iteration_check_2.o benchmark.o
diff --git a/tools/testing/radix-tree/bitmap.c b/tools/testing/radix-tree/bitmap.c
deleted file mode 100644
index 66ec4a24a203..000000000000
--- a/tools/testing/radix-tree/bitmap.c
+++ /dev/null
@@ -1,23 +0,0 @@
-/* lib/bitmap.c pulls in at least two other files. */
-
-#include <linux/bitmap.h>
-
-void bitmap_clear(unsigned long *map, unsigned int start, int len)
-{
-	unsigned long *p = map + BIT_WORD(start);
-	const unsigned int size = start + len;
-	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
-	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
-
-	while (len - bits_to_clear >= 0) {
-		*p &= ~mask_to_clear;
-		len -= bits_to_clear;
-		bits_to_clear = BITS_PER_LONG;
-		mask_to_clear = ~0UL;
-		p++;
-	}
-	if (len) {
-		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
-		*p &= ~mask_to_clear;
-	}
-}
-- 
2.34.1

Re: [PATCH] radix tree test suite: put definition of bitmap_clear() into lib/bitmap.c

Posted by Yury Norov 1 year, 5 months ago

On Wed, Jul 10, 2024 at 09:55:54AM +0000, Wei Yang wrote:
> Instead of keeping a bitmap.c in radix-tree own directory, we can use
> the common implementation in lib/ directory.
> 
> Just move the definition and link some related libs, no functional
> change.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Matthew Wilcox <willy@infradead.org>

Hi Wei,

bitmap_clear() is not used in radix tests, and in fact not used
anywhere in tools at all. Unless you have plans on it, it's
better to drop bitmap_clear() from tools, instead of bloating
the library.

Thanks,
Yury

> ---
>  tools/lib/bitmap.c                | 20 ++++++++++++++++++++
>  tools/testing/radix-tree/Makefile |  4 ++--
>  tools/testing/radix-tree/bitmap.c | 23 -----------------------
>  3 files changed, 22 insertions(+), 25 deletions(-)
>  delete mode 100644 tools/testing/radix-tree/bitmap.c
> 
> diff --git a/tools/lib/bitmap.c b/tools/lib/bitmap.c
> index c3e4871967bc..a96dbf001244 100644
> --- a/tools/lib/bitmap.c
> +++ b/tools/lib/bitmap.c
> @@ -100,3 +100,23 @@ bool __bitmap_intersects(const unsigned long *bitmap1,
>  			return true;
>  	return false;
>  }
> +
> +void bitmap_clear(unsigned long *map, unsigned int start, int len)
> +{
> +	unsigned long *p = map + BIT_WORD(start);
> +	const unsigned int size = start + len;
> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
> +
> +	while (len - bits_to_clear >= 0) {
> +		*p &= ~mask_to_clear;
> +		len -= bits_to_clear;
> +		bits_to_clear = BITS_PER_LONG;
> +		mask_to_clear = ~0UL;
> +		p++;
> +	}
> +	if (len) {
> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
> +		*p &= ~mask_to_clear;
> +	}
> +}
> diff --git a/tools/testing/radix-tree/Makefile b/tools/testing/radix-tree/Makefile
> index 7527f738b4a1..505cc24a8b9c 100644
> --- a/tools/testing/radix-tree/Makefile
> +++ b/tools/testing/radix-tree/Makefile
> @@ -5,8 +5,8 @@ CFLAGS += -I. -I../../include -I../../../lib -g -Og -Wall \
>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>  LDLIBS+= -lpthread -lurcu
>  TARGETS = main idr-test multiorder xarray maple
> -CORE_OFILES := xarray.o radix-tree.o idr.o linux.o test.o find_bit.o bitmap.o \
> -			 slab.o maple.o
> +LIBS := find_bit.o bitmap.o hweight.o vsprintf.o
> +CORE_OFILES := xarray.o radix-tree.o idr.o linux.o test.o slab.o maple.o $(LIBS)
>  OFILES = main.o $(CORE_OFILES) regression1.o regression2.o regression3.o \
>  	 regression4.o tag_check.o multiorder.o idr-test.o iteration_check.o \
>  	 iteration_check_2.o benchmark.o
> diff --git a/tools/testing/radix-tree/bitmap.c b/tools/testing/radix-tree/bitmap.c
> deleted file mode 100644
> index 66ec4a24a203..000000000000
> --- a/tools/testing/radix-tree/bitmap.c
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* lib/bitmap.c pulls in at least two other files. */
> -
> -#include <linux/bitmap.h>
> -
> -void bitmap_clear(unsigned long *map, unsigned int start, int len)
> -{
> -	unsigned long *p = map + BIT_WORD(start);
> -	const unsigned int size = start + len;
> -	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
> -	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
> -
> -	while (len - bits_to_clear >= 0) {
> -		*p &= ~mask_to_clear;
> -		len -= bits_to_clear;
> -		bits_to_clear = BITS_PER_LONG;
> -		mask_to_clear = ~0UL;
> -		p++;
> -	}
> -	if (len) {
> -		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
> -		*p &= ~mask_to_clear;
> -	}
> -}
> -- 
> 2.34.1

Re: [PATCH] radix tree test suite: put definition of bitmap_clear() into lib/bitmap.c

Posted by Wei Yang 1 year, 5 months ago

On Wed, Jul 10, 2024 at 09:17:00AM -0700, Yury Norov wrote:
>On Wed, Jul 10, 2024 at 09:55:54AM +0000, Wei Yang wrote:
>> Instead of keeping a bitmap.c in radix-tree own directory, we can use
>> the common implementation in lib/ directory.
>> 
>> Just move the definition and link some related libs, no functional
>> change.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Matthew Wilcox <willy@infradead.org>
>
>Hi Wei,
>
>bitmap_clear() is not used in radix tests, and in fact not used
>anywhere in tools at all. Unless you have plans on it, it's
>better to drop bitmap_clear() from tools, instead of bloating
>the library.

I guess you didn't try to compile the test. After remove the bitmap_clear(),
we see this error.

cc -fsanitize=address -fsanitize=undefined  main.o xarray.o radix-tree.o idr.o linux.o test.o maple.o slab.o find_bit.o bitmap.o hweight.o vsprintf.o regression1.o regression2.o regression3.o regression4.o tag_check.o multiorder.o idr-test.o iteration_check.o iteration_check_2.o benchmark.o  -lpthread -lurcu -o main
/usr/bin/ld: xarray.o: in function `xas_squash_marks':
/home/richard/git/linux/tools/testing/radix-tree/../../../lib/xarray.c:139: undefined reference to `bitmap_clear'

>
>Thanks,
>Yury
>
>> ---
>>  tools/lib/bitmap.c                | 20 ++++++++++++++++++++
>>  tools/testing/radix-tree/Makefile |  4 ++--
>>  tools/testing/radix-tree/bitmap.c | 23 -----------------------
>>  3 files changed, 22 insertions(+), 25 deletions(-)
>>  delete mode 100644 tools/testing/radix-tree/bitmap.c
>> 
>> diff --git a/tools/lib/bitmap.c b/tools/lib/bitmap.c
>> index c3e4871967bc..a96dbf001244 100644
>> --- a/tools/lib/bitmap.c
>> +++ b/tools/lib/bitmap.c
>> @@ -100,3 +100,23 @@ bool __bitmap_intersects(const unsigned long *bitmap1,
>>  			return true;
>>  	return false;
>>  }
>> +
>> +void bitmap_clear(unsigned long *map, unsigned int start, int len)
>> +{
>> +	unsigned long *p = map + BIT_WORD(start);
>> +	const unsigned int size = start + len;
>> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>> +
>> +	while (len - bits_to_clear >= 0) {
>> +		*p &= ~mask_to_clear;
>> +		len -= bits_to_clear;
>> +		bits_to_clear = BITS_PER_LONG;
>> +		mask_to_clear = ~0UL;
>> +		p++;
>> +	}
>> +	if (len) {
>> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>> +		*p &= ~mask_to_clear;
>> +	}
>> +}
>> diff --git a/tools/testing/radix-tree/Makefile b/tools/testing/radix-tree/Makefile
>> index 7527f738b4a1..505cc24a8b9c 100644
>> --- a/tools/testing/radix-tree/Makefile
>> +++ b/tools/testing/radix-tree/Makefile
>> @@ -5,8 +5,8 @@ CFLAGS += -I. -I../../include -I../../../lib -g -Og -Wall \
>>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>>  LDLIBS+= -lpthread -lurcu
>>  TARGETS = main idr-test multiorder xarray maple
>> -CORE_OFILES := xarray.o radix-tree.o idr.o linux.o test.o find_bit.o bitmap.o \
>> -			 slab.o maple.o
>> +LIBS := find_bit.o bitmap.o hweight.o vsprintf.o
>> +CORE_OFILES := xarray.o radix-tree.o idr.o linux.o test.o slab.o maple.o $(LIBS)
>>  OFILES = main.o $(CORE_OFILES) regression1.o regression2.o regression3.o \
>>  	 regression4.o tag_check.o multiorder.o idr-test.o iteration_check.o \
>>  	 iteration_check_2.o benchmark.o
>> diff --git a/tools/testing/radix-tree/bitmap.c b/tools/testing/radix-tree/bitmap.c
>> deleted file mode 100644
>> index 66ec4a24a203..000000000000
>> --- a/tools/testing/radix-tree/bitmap.c
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/* lib/bitmap.c pulls in at least two other files. */
>> -
>> -#include <linux/bitmap.h>
>> -
>> -void bitmap_clear(unsigned long *map, unsigned int start, int len)
>> -{
>> -	unsigned long *p = map + BIT_WORD(start);
>> -	const unsigned int size = start + len;
>> -	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>> -	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>> -
>> -	while (len - bits_to_clear >= 0) {
>> -		*p &= ~mask_to_clear;
>> -		len -= bits_to_clear;
>> -		bits_to_clear = BITS_PER_LONG;
>> -		mask_to_clear = ~0UL;
>> -		p++;
>> -	}
>> -	if (len) {
>> -		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>> -		*p &= ~mask_to_clear;
>> -	}
>> -}
>> -- 
>> 2.34.1

-- 
Wei Yang
Help you, Help me

Re: [PATCH] radix tree test suite: put definition of bitmap_clear() into lib/bitmap.c

Posted by Yury Norov 1 year, 5 months ago

On Wed, Jul 10, 2024 at 04:31:34PM +0000, Wei Yang wrote:
> On Wed, Jul 10, 2024 at 09:17:00AM -0700, Yury Norov wrote:
> >On Wed, Jul 10, 2024 at 09:55:54AM +0000, Wei Yang wrote:
> >> Instead of keeping a bitmap.c in radix-tree own directory, we can use
> >> the common implementation in lib/ directory.
> >> 
> >> Just move the definition and link some related libs, no functional
> >> change.
> >> 
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >> CC: Matthew Wilcox <willy@infradead.org>
> >
> >Hi Wei,
> >
> >bitmap_clear() is not used in radix tests, and in fact not used
> >anywhere in tools at all. Unless you have plans on it, it's
> >better to drop bitmap_clear() from tools, instead of bloating
> >the library.
> 
> I guess you didn't try to compile the test. After remove the bitmap_clear(),
> we see this error.

No, I didn't.
 
> cc -fsanitize=address -fsanitize=undefined  main.o xarray.o radix-tree.o idr.o linux.o test.o maple.o slab.o find_bit.o bitmap.o hweight.o vsprintf.o regression1.o regression2.o regression3.o regression4.o tag_check.o multiorder.o idr-test.o iteration_check.o iteration_check_2.o benchmark.o  -lpthread -lurcu -o main
> /usr/bin/ld: xarray.o: in function `xas_squash_marks':
> /home/richard/git/linux/tools/testing/radix-tree/../../../lib/xarray.c:139: undefined reference to `bitmap_clear'

Ok then. If xarray userspace test pulls code directly from kernel
part, we still need bitmap_clear.

Since the function was introduced, we wrote an inline version of
it. We usually try to keep tools implementation synced with the
mother kernel, particularly for extra testing coverage.

Can you please send a v2 that syncs tools implementation with the
kernel one, and can you also mention how it is used in the code,
so those lazy folks like me will not get fooled with grep.

Thanks,
Yury