bitmap: Add bitmap_zero()/bitmap_clear() test cases

Behaviour of bitmap_fill() differs from bitmap_zero() in a way how bits behind bitmap are handed. bitmap_zero() clears entire bitmap by unsigned long boundary, while bitmap_fill() mimics bitmap_set().

Here we change bitmap_fill() behaviour to be consistent with bitmap_zero() and add a note to documentation.

The change might reveal some bugs in the code where unused bits handled differently and in such cases bitmap_set() has to be used.

Suggested-by Rasmus Villemoes <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 include/linux/bitmap.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

include/linux/bitmap.h +10 -5

index a17b5121d4f5..49aea0b37994 100644
@@ -67,6 +67,11 @@
  *  bitmap_from_arr32(dst, buf, nbits)          Copy nbits from u32[] buf to dst
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
  *
+ * Note, bitmap_zero() and bitmap_fill() operate over the region of
+ * unsigned longs, that is, bits behind bitmap till the unsigned long
+ * boundary will be zeroed or filled as well. Consider to use
+ * bitmap_clear() or bitmap_set() to make explicit zeroing or filling
+ * respectively.
  */
 
 /**

@@ -206,12 +211,12 @@ static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
 
 static inline void bitmap_fill(unsigned long *dst, unsigned int nbits)
 {
-	unsigned int nlongs = BITS_TO_LONGS(nbits);
-	if (!small_const_nbits(nbits)) {
-		unsigned int len = (nlongs - 1) * sizeof(unsigned long);
-		memset(dst, 0xff,  len);
+	if (small_const_nbits(nbits))
+		*dst = ~0UL;
+	else {
+		unsigned int len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
+		memset(dst, 0xff, len);
 	}
-	dst[nlongs - 1] = BITMAP_LAST_WORD_MASK(nbits);
 }
 
 static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,

Signature · 1 line

2.15.1

Quoted 9 lines · Andy Shevchenko

Behaviour of bitmap_fill() differs from bitmap_zero() in a way how bits behind bitmap are handed. bitmap_zero() clears entire bitmap
by unsigned long boundary, while bitmap_fill() mimics bitmap_set().

Here we change bitmap_fill() behaviour to be consistent with bitmap_zero()
and add a note to documentation.

Quoted 3 lines

The change might reveal some bugs in the code where unused bits handled
differently and in such cases bitmap_set() has to be used.

There is only 51 users of bitmap_fill() in the kernel, including tests. If you propose this change, I think you'd check them all manually.

Sorry that.

Also, there's tools/include/linux/bitmap.h which has a copy of bitmap_fill(), and should be consistent with main kernel sources.

Quoted 10 lines · Yury Norov

Quoted 9 lines · Andy Shevchenko

Behaviour of bitmap_fill() differs from bitmap_zero() in a way how bits behind bitmap are handed. bitmap_zero() clears entire bitmap
by unsigned long boundary, while bitmap_fill() mimics bitmap_set().

Here we change bitmap_fill() behaviour to be consistent with bitmap_zero()
and add a note to documentation.

Quoted 7 lines

Quoted 3 lines

The change might reveal some bugs in the code where unused bits handled
differently and in such cases bitmap_set() has to be used.

There is only 51 users of bitmap_fill() in the kernel, including tests. If you propose this change, I think you'd check them all manually.

Some of them might require 5 minutes to check while others (especially in the areas I don't know much about) 5+ hours. I rely on Rasmus assumption that there _were_ bugs, though they assumed to be fixed by now.

In any case I'm ready to take responsibility of possible breakage and fully into provide fixes by demand.

Quoted 1 line

Sorry that.

I lost your thought here. What did you mean by this?

Quoted 2 lines

Also, there's tools/include/linux/bitmap.h which has a copy of bitmap_fill(), and should be consistent with main kernel sources.

tools is independent, although quite related, project to the kernel itself. They will decide by themselves how to proceed, I suppose.

At least what I see in the history of changes in the tools/ they usually follow the changes in main library after while.

Quoted 18 lines · Andy Shevchenko

Quoted 9 lines · Yury Norov

Quoted 3 lines · Andy Shevchenko

The change might reveal some bugs in the code where unused bits handled
differently and in such cases bitmap_set() has to be used.

There is only 51 users of bitmap_fill() in the kernel, including tests. If you propose this change, I think you'd check them all manually.

Some of them might require 5 minutes to check while others (especially
in the areas I don't know much about) 5+ hours. I rely on Rasmus assumption that there _were_ bugs, though they assumed to be fixed by
now.

Quoted 3 lines

In any case I'm ready to take responsibility of possible breakage and
fully into provide fixes by demand.

Is my understanding correct that you need almost a working day to decide what function to use - bitmap_set() or bitmap_fill() in some cases,

and there are at least 2 cases like that?

If so, there's quite realistic chance that bug will hit us 6 month after upstreaming the patch when affected kernel will be delivered to end users by distro developers.

This is not acceptable scenario. If you have willing to take responsibility, please do it now and don't wait when things go broken.

Quoted 3 lines

Quoted 1 line

Sorry that.

I lost your thought here. What did you mean by this?

I only mean that I realize that I ask you to do big amount of boring mechanical work, and I'm not happy with that.

Quoted 6 lines

Quoted 2 lines

Also, there's tools/include/linux/bitmap.h which has a copy of bitmap_fill(), and should be consistent with main kernel sources.

tools is independent, although quite related, project to the kernel itself. They will decide by themselves how to proceed, I suppose.

Quoted 3 lines

At least what I see in the history of changes in the tools/ they usually
follow the changes in main library after while.

[CC Arnaldo Carvalho de Melo <[email protected]>]

You can always ask tools/* maintainers what is better for them.

For me,
people simply forget about tools/* and that's why maintainers have to sync sources periodically.

Anyway, if you think that your change is good
enough for Linux kernel, why don't you think so for tools?

Explicitly test bitmap_zero() and bitmap_clear() functions.

Signed-off-by Andy Shevchenko <[email protected]>

---
 lib/test_bitmap.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

lib/test_bitmap.c +0 -0

index de7ef2996a07..9734af711816 100644

lib/test_bitmap.c +24 -0

+	memset(bmap, 0xff, 128);
+
+	expect_eq_pbl("0-22", bmap, 23);
+	expect_eq_pbl("0-1023", bmap, 1024);
+
+	/* single-word bitmaps */
+	bitmap_clear(bmap, 0, 9);
+	expect_eq_pbl("9-1023", bmap, 1024);
+
+	bitmap_zero(bmap, 35);
+	expect_eq_pbl("64-1023", bmap, 1024);
+
+	/* cross boundaries operations */
+	bitmap_clear(bmap, 79, 19);
+	expect_eq_pbl("64-78,98-1023", bmap, 1024);
+
+	bitmap_zero(bmap, 115);
+	expect_eq_pbl("128-1023", bmap, 1024);
+
+	/* Zeroing entire area */
+	bitmap_zero(bmap, 1024);
+	expect_eq_pbl("", bmap, 1024);
+}
+
 static void __init test_zero_fill_copy(void)
 {
 	DECLARE_BITMAP(bmap1, 1024);

Hi Andy,

lib/test_bitmap.c __check_eq_u32_array(const char *srcfile, unsigned int line, · line 112

Quoted 3 lines · Andy Shevchenko

Explicitly test bitmap_zero() and bitmap_clear() functions.

lib/test_bitmap.c +1 -0

+	/* Known way to set all bits */

Nit: if you start your comments with capital, proceed that way till the end.

I don't understand what patch #4 is doing in this series. At the first glance, it may be applied separately.

The rest is looking fine. For 1-3,

Reviewed-by Yury Norov <[email protected]>

Quoted 9 lines · Yury Norov

Hi Andy,

lib/test_bitmap.c __check_eq_u32_array(const char *srcfile, unsigned int line, · line 112

Quoted 3 lines · Andy Shevchenko

Explicitly test bitmap_zero() and bitmap_clear() functions.

lib/test_bitmap.c +1 -0

+	/* Known way to set all bits */

Nit: if you start your comments with capital, proceed that way till the end.

Right, I have to keep the original style. I'll check this.

Quoted 2 lines

I don't understand what patch #4 is doing in this series. At the first glance, it may be applied separately.

It fixes test failures found by patch 2 in the series. The idea is similar to TDD.

Quoted 7 lines · Andy Shevchenko

Quoted 5 lines · Yury Norov

I don't understand what patch #4 is doing in this series. At the first
glance, it may be applied separately.

Quoted 2 lines

It fixes test failures found by patch 2 in the series. The idea is similar to TDD.

So with current order, patch 2 introduces regression that is fixed in patch 4, is my understanding correct?

This is not the best idea because it will break bisectability.