[PATCH v1 1/4] bitmap: Add bitmap_zero()/bitmap_clear() test cases

[PATCH v1 1/4] bitmap: Add bitmap_zero()/bitmap_clear() test cases

[Andy Shevchenko]

Explicitly test bitmap_zero() and bitmap_clear() functions.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_bitmap.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index de7ef2996a07..9734af711816 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -105,6 +105,35 @@ __check_eq_u32_array(const char *srcfile, unsigned int line,
 #define expect_eq_pbl(...)		__expect_eq(pbl, ##__VA_ARGS__)
 #define expect_eq_u32_array(...)	__expect_eq(u32_array, ##__VA_ARGS__)
 
+static void __init test_zero_clear(void)
+{
+	DECLARE_BITMAP(bmap, 1024);
+
+	/* Known way to set all bits */
+	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);
@@ -309,6 +338,7 @@ static void noinline __init test_mem_optimisations(void)
 
 static int __init test_bitmap_init(void)
 {
+	test_zero_clear();
 	test_zero_fill_copy();
 	test_bitmap_arr32();
 	test_bitmap_parselist();
-- 
2.15.1

[PATCH v1 2/4] bitmap: Add bitmap_fill()/bitmap_set() test cases

[Andy Shevchenko]

Explicitly test bitmap_fill() and bitmap_set() functions.

For bitmap_fill() we expect a consistent behaviour as in bitmap_zero(),
i.e.  the trailing bits will be set up to unsigned long boundary.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_bitmap.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 9734af711816..6889fcc0e1f4 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -134,6 +134,35 @@ static void __init test_zero_clear(void)
 	expect_eq_pbl("", bmap, 1024);
 }
 
+static void __init test_fill_set(void)
+{
+	DECLARE_BITMAP(bmap, 1024);
+
+	/* Known way to clear all bits */
+	memset(bmap, 0x00, 128);
+
+	expect_eq_pbl("", bmap, 23);
+	expect_eq_pbl("", bmap, 1024);
+
+	/* single-word bitmaps */
+	bitmap_set(bmap, 0, 9);
+	expect_eq_pbl("0-8", bmap, 1024);
+
+	bitmap_fill(bmap, 35);
+	expect_eq_pbl("0-63", bmap, 1024);
+
+	/* cross boundaries operations */
+	bitmap_set(bmap, 79, 19);
+	expect_eq_pbl("0-63,79-97", bmap, 1024);
+
+	bitmap_fill(bmap, 115);
+	expect_eq_pbl("0-127", bmap, 1024);
+
+	/* Zeroing entire area */
+	bitmap_fill(bmap, 1024);
+	expect_eq_pbl("0-1023", bmap, 1024);
+}
+
 static void __init test_zero_fill_copy(void)
 {
 	DECLARE_BITMAP(bmap1, 1024);
@@ -339,6 +368,7 @@ static void noinline __init test_mem_optimisations(void)
 static int __init test_bitmap_init(void)
 {
 	test_zero_clear();
+	test_fill_set();
 	test_zero_fill_copy();
 	test_bitmap_arr32();
 	test_bitmap_parselist();
-- 
2.15.1

[PATCH v1 3/4] bitmap: Clean up test_zero_fill_copy() test case and rename

[Andy Shevchenko]

Since we have separate explicit test cases for bitmap_zero() / bitmap_clear()
and bitmap_fill() / bitmap_set(), clean up test_zero_fill_copy() to only test
bitmap_copy() functionality and thus rename a function to reflect the changes.

While here, replace bitmap_fill() by bitmap_set() with proper values.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_bitmap.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 6889fcc0e1f4..b3f235baa05d 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -163,7 +163,7 @@ static void __init test_fill_set(void)
 	expect_eq_pbl("0-1023", bmap, 1024);
 }
 
-static void __init test_zero_fill_copy(void)
+static void __init test_copy(void)
 {
 	DECLARE_BITMAP(bmap1, 1024);
 	DECLARE_BITMAP(bmap2, 1024);
@@ -172,36 +172,20 @@ static void __init test_zero_fill_copy(void)
 	bitmap_zero(bmap2, 1024);
 
 	/* single-word bitmaps */
-	expect_eq_pbl("", bmap1, 23);
-
-	bitmap_fill(bmap1, 19);
-	expect_eq_pbl("0-18", bmap1, 1024);
-
+	bitmap_set(bmap1, 0, 19);
 	bitmap_copy(bmap2, bmap1, 23);
 	expect_eq_pbl("0-18", bmap2, 1024);
 
-	bitmap_fill(bmap2, 23);
-	expect_eq_pbl("0-22", bmap2, 1024);
-
+	bitmap_set(bmap2, 0, 23);
 	bitmap_copy(bmap2, bmap1, 23);
 	expect_eq_pbl("0-18", bmap2, 1024);
 
-	bitmap_zero(bmap1, 23);
-	expect_eq_pbl("", bmap1, 1024);
-
 	/* multi-word bitmaps */
-	bitmap_zero(bmap1, 1024);
-	expect_eq_pbl("", bmap1, 1024);
-
-	bitmap_fill(bmap1, 109);
-	expect_eq_pbl("0-108", bmap1, 1024);
-
+	bitmap_set(bmap1, 0, 109);
 	bitmap_copy(bmap2, bmap1, 1024);
 	expect_eq_pbl("0-108", bmap2, 1024);
 
 	bitmap_fill(bmap2, 1024);
-	expect_eq_pbl("0-1023", bmap2, 1024);
-
 	bitmap_copy(bmap2, bmap1, 1024);
 	expect_eq_pbl("0-108", bmap2, 1024);
 
@@ -216,9 +200,6 @@ static void __init test_zero_fill_copy(void)
 	bitmap_fill(bmap2, 1024);
 	bitmap_copy(bmap2, bmap1, 97);  /* ... but aligned on word length */
 	expect_eq_pbl("0-108,128-1023", bmap2, 1024);
-
-	bitmap_zero(bmap2, 97);  /* ... but 0-padded til word length */
-	expect_eq_pbl("128-1023", bmap2, 1024);
 }
 
 #define PARSE_TIME 0x1
@@ -369,7 +350,7 @@ static int __init test_bitmap_init(void)
 {
 	test_zero_clear();
 	test_fill_set();
-	test_zero_fill_copy();
+	test_copy();
 	test_bitmap_arr32();
 	test_bitmap_parselist();
 	test_mem_optimisations();
-- 
2.15.1

[PATCH v1 4/4] bitmap: Make bitmap_fill() and bitmap_zero() consistent

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

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 <linux@rasmusvillemoes.dk>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/bitmap.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index a17b5121d4f5..49aea0b37994 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -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,
-- 
2.15.1

Re: [PATCH v1 4/4] bitmap: Make bitmap_fill() and bitmap_zero() consistent

[Yury Norov]

On Tue, Jan 09, 2018 at 07:24:30PM +0200, Andy Shevchenko wrote:
> 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.

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.

> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/bitmap.h | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index a17b5121d4f5..49aea0b37994 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -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,
> -- 
> 2.15.1

Re: [PATCH v1 4/4] bitmap: Make bitmap_fill() and bitmap_zero() consistent

[Andy Shevchenko]

On Wed, 2018-01-10 at 11:49 +0300, Yury Norov wrote:
> On Tue, Jan 09, 2018 at 07:24:30PM +0200, Andy Shevchenko wrote:
> > 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.
> 
> 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.

>  Sorry that.

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

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

Thanks for review!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 4/4] bitmap: Make bitmap_fill() and bitmap_zero() consistent

[Yury Norov]

On Wed, Jan 10, 2018 at 03:17:03PM +0200, Andy Shevchenko wrote:
> On Wed, 2018-01-10 at 11:49 +0300, Yury Norov wrote:
> > On Tue, Jan 09, 2018 at 07:24:30PM +0200, Andy Shevchenko wrote:
> > > 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.
> > 
> > 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.

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.

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

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

[CC Arnaldo Carvalho de Melo <acme@redhat.com>]

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?

Yury

Re: [PATCH v1 4/4] bitmap: Make bitmap_fill() and bitmap_zero() consistent

[Andy Shevchenko]

On Thu, 2018-01-11 at 14:57 +0300, Yury Norov wrote:
> On Wed, Jan 10, 2018 at 03:17:03PM +0200, Andy Shevchenko wrote:
> > On Wed, 2018-01-10 at 11:49 +0300, Yury Norov wrote:
> > > On Tue, Jan 09, 2018 at 07:24:30PM +0200, Andy Shevchenko wrote:

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

I don't know. There are like you said 51 user of the bitmap_fill().

If we lucky that developers are not so-o-o dumb to use bitmap_fill() as
a replacement of bitmap_set(..., 0, ...), nothing will need to be fixed.

>  and there are at least 2 cases like that?

Where this come from?

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

It would be found much earlier in the core code, otherwise it's business
as usual.

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

So, instead of beating the air you can help to check the places, right?

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

It's not mechanical, that is the point. (Incorrect) usage of
bitmap_fill() is a bug. Fix that helps to reveal it earlier is a good
one.

> > > 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.
> 
> [CC Arnaldo Carvalho de Melo <acme@redhat.com>]
> 
> You can always ask tools/* maintainers what is better for them.

Yeah, notification is a good thing.

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

Might be, my at least one patch (and few pings) to tools code at the end
is left neither commented not applied for years, so, I gave up on them,
sorry @acme et al. OTOH fixes for Makefiles are usually go in quickly.

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

I didn't tell that.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/4] bitmap: Add bitmap_zero()/bitmap_clear() test cases

[Yury Norov]

Hi Andy,

On Tue, Jan 09, 2018 at 07:24:27PM +0200, Andy Shevchenko wrote:
> Explicitly test bitmap_zero() and bitmap_clear() functions.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_bitmap.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index de7ef2996a07..9734af711816 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -105,6 +105,35 @@ __check_eq_u32_array(const char *srcfile, unsigned int line,
>  #define expect_eq_pbl(...)		__expect_eq(pbl, ##__VA_ARGS__)
>  #define expect_eq_u32_array(...)	__expect_eq(u32_array, ##__VA_ARGS__)
>  
> +static void __init test_zero_clear(void)
> +{
> +	DECLARE_BITMAP(bmap, 1024);
> +
> +	/* Known way to set all bits */

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

> +	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);
> @@ -309,6 +338,7 @@ static void noinline __init test_mem_optimisations(void)
>  
>  static int __init test_bitmap_init(void)
>  {
> +	test_zero_clear();
>  	test_zero_fill_copy();
>  	test_bitmap_arr32();
>  	test_bitmap_parselist();

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 <ynorov@caviumnetworks.com>

Re: [PATCH v1 1/4] bitmap: Add bitmap_zero()/bitmap_clear() test cases

[Andy Shevchenko]

On Wed, 2018-01-10 at 12:34 +0300, Yury Norov wrote:
> Hi Andy,
> 
> On Tue, Jan 09, 2018 at 07:24:27PM +0200, Andy Shevchenko wrote:
> > Explicitly test bitmap_zero() and bitmap_clear() functions.

> > +	/* 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.

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

> The rest is looking fine. For 1-3,
> Reviewed-by: Yury Norov <ynorov@caviumnetworks.com>

Thanks.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/4] bitmap: Add bitmap_zero()/bitmap_clear() test cases

[Yury Norov]

On Wed, Jan 10, 2018 at 03:11:45PM +0200, Andy Shevchenko wrote:
> On Wed, 2018-01-10 at 12:34 +0300, Yury Norov wrote:
> > Hi Andy,
> > 
> > On Tue, Jan 09, 2018 at 07:24:27PM +0200, Andy Shevchenko wrote:
> > > Explicitly test bitmap_zero() and bitmap_clear() functions.
> 
> > > +	/* 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.
> 
> > 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.

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. I would
recommend you to change the order and move patch #4 to the begin.

Also it would be reasonable to leave a note in patch 2 comment that it
causes regression if applied alone.

Yury

Re: [PATCH v1 1/4] bitmap: Add bitmap_zero()/bitmap_clear() test cases

[Andy Shevchenko]

On Thu, 2018-01-11 at 15:07 +0300, Yury Norov wrote:
> On Wed, Jan 10, 2018 at 03:11:45PM +0200, Andy Shevchenko wrote:
> > On Wed, 2018-01-10 at 12:34 +0300, Yury Norov wrote:
> > > 

> > > 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.
> 
> So with current order, patch 2 introduces regression that is fixed in
> patch 4, is my understanding correct?

I'm sorry to ask, but do you call new test cases "a regression" for
real?!

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

Huh?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy