[PATCH v2 0/5] bitmap: fix conversion from/to fix-sized arrays

[PATCH v2 0/5] bitmap: fix conversion from/to fix-sized arrays

[Yury Norov]

In the kernel codebase we have functions that call bitmap_copy()
to convert bitmaps to and from fix-sized 32 or 64-bit arrays. It
works well for LE architectures when size of long is equal to the
size of fixed type.

If the system is BE and/or size of long is not equal to the size of
fixed type of the array, bitmap_copy() may produce wrong result either
because of endianness issue, or because of out-of-bound access.

To address this problem we have bitmap_{from,to}_arr32(). In recent
discussion it was spotted that we also need 64-bit analogue:

https://lore.kernel.org/all/YiCWNdWd+AsLbDkp@smile.fi.intel.com/T/#m754da92acb0003e12b99293d07ddcd46dbe04ada

This series takes care of it.

v1: https://lore.kernel.org/lkml/20220420222530.910125-3-yury.norov@gmail.com/T/
v2: - fix build warnings (patch 2)
    - add test for bitmap_{from,to}_arr64

Yury Norov (5):
  lib/bitmap: extend comment for bitmap_(from,to)_arr32()
  lib: add bitmap_{from,to}_arr64
  lib/bitmap: add test for bitmap_{from,to}_arr64
  KVM: s390: replace bitmap_copy with bitmap_{from,to}_arr64 where
    appropriate
  drm/amd/pm: use bitmap_{from,to}_arr32 where appropriate

 arch/s390/kvm/kvm-s390.c                      | 10 ++--
 .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c    |  2 +-
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +-
 include/linux/bitmap.h                        | 31 +++++++++---
 lib/bitmap.c                                  | 48 +++++++++++++++++++
 lib/test_bitmap.c                             | 25 ++++++++++
 6 files changed, 103 insertions(+), 15 deletions(-)

-- 
2.32.0

[PATCH 1/5] lib/bitmap: extend comment for bitmap_(from,to)_arr32()

[Yury Norov]

On LE systems bitmaps are naturally ordered, therefore we can potentially
use bitmap_copy routines when converting from 32-bit arrays, even if host
system is 64-bit. But it may lead to out-of-bond access due to unsafe
typecast, and the bitmap_(from,to)_arr32 comment doesn't explain that
clearly

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 983dc3f2d57b..dbdf1685debf 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -271,8 +271,12 @@ static inline void bitmap_copy_clear_tail(unsigned long *dst,
 }
 
 /*
- * On 32-bit systems bitmaps are represented as u32 arrays internally, and
- * therefore conversion is not needed when copying data from/to arrays of u32.
+ * On 32-bit systems bitmaps are represented as u32 arrays internally. On LE64
+ * machines the order of hi and lo parts of numbers match the bitmap structure.
+ * In both cases conversion is not needed when copying data from/to arrays of
+ * u32. But in LE64 case, typecast in bitmap_copy_clear_tail() may lead
+ * to out-of-bound access. To avoid that, both LE and BE variants of 64-bit
+ * architectures are not using bitmap_copy_clear_tail().
  */
 #if BITS_PER_LONG == 64
 void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
-- 
2.32.0

[PATCH 2/5] lib: add bitmap_{from,to}_arr64

[Yury Norov]

Manipulating 64-bit arrays with bitmap functions is potentially dangerous
because on 32-bit BE machines the order of halfwords doesn't match.
Another issue is that compiler may throw a warning about out-of-boundary
access.

This patch adds bitmap_{from,to}_arr64 functions in addition to existing
bitmap_{from,to}_arr32.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 23 ++++++++++++++++----
 lib/bitmap.c           | 48 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index dbdf1685debf..57f1c74239d5 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -292,6 +292,24 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
 			(const unsigned long *) (bitmap), (nbits))
 #endif
 
+/*
+ * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32
+ * machines the order of hi and lo parts of numbers match the bitmap structure.
+ * In both cases conversion is not needed when copying data from/to arrays of
+ * u64.
+ */
+#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
+void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits);
+void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits);
+#else
+#define bitmap_from_arr64(bitmap, buf, nbits)			\
+	bitmap_copy_clear_tail((unsigned long *) (bitmap),	\
+			(const unsigned long *) (buf), (nbits))
+#define bitmap_to_arr64(buf, bitmap, nbits)			\
+	bitmap_copy_clear_tail((unsigned long *) (buf),		\
+			(const unsigned long *) (bitmap), (nbits))
+#endif
+
 static inline int bitmap_and(unsigned long *dst, const unsigned long *src1,
 			const unsigned long *src2, unsigned int nbits)
 {
@@ -601,10 +619,7 @@ static inline void bitmap_next_set_region(unsigned long *bitmap,
  */
 static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
 {
-	dst[0] = mask & ULONG_MAX;
-
-	if (sizeof(mask) > sizeof(unsigned long))
-		dst[1] = mask >> 32;
+	bitmap_from_arr64(dst, &mask, 64);
 }
 
 /**
diff --git a/lib/bitmap.c b/lib/bitmap.c
index d9a4480af5b9..027b63a655fd 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1533,5 +1533,53 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
 		buf[halfwords - 1] &= (u32) (UINT_MAX >> ((-nbits) & 31));
 }
 EXPORT_SYMBOL(bitmap_to_arr32);
+#endif
+
+#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
+/**
+ * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap
+ *	@bitmap: array of unsigned longs, the destination bitmap
+ *	@buf: array of u64 (in host byte order), the source bitmap
+ *	@nbits: number of bits in @bitmap
+ */
+void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits)
+{
+	int n;
+
+	for (n = nbits; n > 0; n -= 64) {
+		u64 val = *buf++;
+
+		*bitmap++ = val;
+		if (n > 32)
+			*bitmap++ = val >> 32;
+	}
+
+	/* Clear tail bits in last word beyond nbits. */
+	if (nbits % BITS_PER_LONG)
+		bitmap[-1] &= BITMAP_LAST_WORD_MASK(nbits);
+}
+EXPORT_SYMBOL(bitmap_from_arr64);
+
+/**
+ * bitmap_to_arr64 - copy the contents of bitmap to a u64 array of bits
+ *	@buf: array of u64 (in host byte order), the dest bitmap
+ *	@bitmap: array of unsigned longs, the source bitmap
+ *	@nbits: number of bits in @bitmap
+ */
+void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
+{
+	const unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
+
+	while (bitmap < end) {
+		*buf = *bitmap++;
+		if (bitmap < end)
+			*buf |= (u64)(*bitmap++) << 32;
+		buf++;
+	}
 
+	/* Clear tail bits in last element of array beyond nbits. */
+	if (nbits % 64)
+		buf[-1] &= GENMASK_ULL(nbits, 0);
+}
+EXPORT_SYMBOL(bitmap_to_arr64);
 #endif
-- 
2.32.0

[PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

Test newly added bitmap_{from,to}_arr64() functions similarly to
already existing bitmap_{from,to}_arr32() tests.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 lib/test_bitmap.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 0c82f07f74fc..d5923a640457 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -585,6 +585,30 @@ static void __init test_bitmap_arr32(void)
 	}
 }
 
+static void __init test_bitmap_arr64(void)
+{
+	unsigned int nbits, next_bit;
+	u64 arr[EXP1_IN_BITS / 64];
+	DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
+
+	memset(arr, 0xa5, sizeof(arr));
+
+	for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
+		memset(bmap2, 0xff, sizeof(arr));
+		bitmap_to_arr64(arr, exp1, nbits);
+		bitmap_from_arr64(bmap2, arr, nbits);
+		expect_eq_bitmap(bmap2, exp1, nbits);
+
+		next_bit = find_next_bit(bmap2, round_up(nbits, BITS_PER_LONG), nbits);
+		if (next_bit < round_up(nbits, BITS_PER_LONG))
+			pr_err("bitmap_copy_arr64(nbits == %d:"
+				" tail is not safely cleared: %d\n", nbits, next_bit);
+
+		if (nbits < EXP1_IN_BITS - 64)
+			expect_eq_uint(arr[DIV_ROUND_UP(nbits, 64)], 0xa5a5a5a5);
+	}
+}
+
 static void noinline __init test_mem_optimisations(void)
 {
 	DECLARE_BITMAP(bmap1, 1024);
@@ -852,6 +876,7 @@ static void __init selftest(void)
 	test_copy();
 	test_replace();
 	test_bitmap_arr32();
+	test_bitmap_arr64();
 	test_bitmap_parse();
 	test_bitmap_parselist();
 	test_bitmap_printlist();
-- 
2.32.0

[PATCH 4/5] KVM: s390: replace bitmap_copy with bitmap_{from,to}_arr64 where appropriate

[Yury Norov]

Copying bitmaps from/to 64-bit arrays with bitmap_copy is not safe
on 32-bit BE machines. Use designated functions instead.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 76ad6408cb2c..8fcb56141689 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1332,8 +1332,7 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
 		mutex_unlock(&kvm->lock);
 		return -EBUSY;
 	}
-	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
-		    KVM_S390_VM_CPU_FEAT_NR_BITS);
+	bitmap_from_arr64(kvm->arch.cpu_feat, data.feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	mutex_unlock(&kvm->lock);
 	VM_EVENT(kvm, 3, "SET: guest feat: 0x%16.16llx.0x%16.16llx.0x%16.16llx",
 			 data.feat[0],
@@ -1504,8 +1503,7 @@ static int kvm_s390_get_processor_feat(struct kvm *kvm,
 {
 	struct kvm_s390_vm_cpu_feat data;
 
-	bitmap_copy((unsigned long *) data.feat, kvm->arch.cpu_feat,
-		    KVM_S390_VM_CPU_FEAT_NR_BITS);
+	bitmap_to_arr64(data.feat, kvm->arch.cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	if (copy_to_user((void __user *)attr->addr, &data, sizeof(data)))
 		return -EFAULT;
 	VM_EVENT(kvm, 3, "GET: guest feat: 0x%16.16llx.0x%16.16llx.0x%16.16llx",
@@ -1520,9 +1518,7 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm,
 {
 	struct kvm_s390_vm_cpu_feat data;
 
-	bitmap_copy((unsigned long *) data.feat,
-		    kvm_s390_available_cpu_feat,
-		    KVM_S390_VM_CPU_FEAT_NR_BITS);
+	bitmap_to_arr64(data.feat, kvm_s390_available_cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	if (copy_to_user((void __user *)attr->addr, &data, sizeof(data)))
 		return -EFAULT;
 	VM_EVENT(kvm, 3, "GET: host feat:  0x%16.16llx.0x%16.16llx.0x%16.16llx",
-- 
2.32.0

[PATCH 5/5] drm/amd/pm: use bitmap_{from,to}_arr32 where appropriate

[Yury Norov]

The smu_v1X_0_set_allowed_mask() uses bitmap_copy() to convert
bitmap to 32-bit array. This may be wrong due to endiannes issues.
Fix it by switching to bitmap_{from,to}_arr32.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 2 +-
 drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
index b87f550af26b..5f8809f6990d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
@@ -781,7 +781,7 @@ int smu_v11_0_set_allowed_mask(struct smu_context *smu)
 		goto failed;
 	}
 
-	bitmap_copy((unsigned long *)feature_mask, feature->allowed, 64);
+	bitmap_to_arr32(feature_mask, feature->allowed, 64);
 
 	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetAllowedFeaturesMaskHigh,
 					  feature_mask[1], NULL);
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
index cf09e30bdfe0..747430ce6394 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
@@ -730,7 +730,7 @@ int smu_v13_0_set_allowed_mask(struct smu_context *smu)
 	    feature->feature_num < 64)
 		return -EINVAL;
 
-	bitmap_copy((unsigned long *)feature_mask, feature->allowed, 64);
+	bitmap_to_arr32(feature_mask, feature->allowed, 64);
 
 	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetAllowedFeaturesMaskHigh,
 					      feature_mask[1], NULL);
-- 
2.32.0

Re: [PATCH 2/5] lib: add bitmap_{from,to}_arr64

[Andy Shevchenko]

On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote:
> Manipulating 64-bit arrays with bitmap functions is potentially dangerous
> because on 32-bit BE machines the order of halfwords doesn't match.
> Another issue is that compiler may throw a warning about out-of-boundary
> access.
> 
> This patch adds bitmap_{from,to}_arr64 functions in addition to existing
> bitmap_{from,to}_arr32.

...

> +	bitmap_copy_clear_tail((unsigned long *) (bitmap),	\
> +			(const unsigned long *) (buf), (nbits))

Drop spaces after castings. Besides that it might be placed on a single line.

...


> +	bitmap_copy_clear_tail((unsigned long *) (buf),		\
> +			(const unsigned long *) (bitmap), (nbits))

Ditto.

...

> +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
> +{
> +	const unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
> +
> +	while (bitmap < end) {
> +		*buf = *bitmap++;
> +		if (bitmap < end)
> +			*buf |= (u64)(*bitmap++) << 32;
> +		buf++;
> +	}
>  
> +	/* Clear tail bits in last element of array beyond nbits. */
> +	if (nbits % 64)
> +		buf[-1] &= GENMASK_ULL(nbits, 0);

Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since
end == bitmap? Or did I miss something?

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] lib: add bitmap_{from,to}_arr64

[Yury Norov]

On Fri, Apr 29, 2022 at 03:59:25PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote:
> > Manipulating 64-bit arrays with bitmap functions is potentially dangerous
> > because on 32-bit BE machines the order of halfwords doesn't match.
> > Another issue is that compiler may throw a warning about out-of-boundary
> > access.
> > 
> > This patch adds bitmap_{from,to}_arr64 functions in addition to existing
> > bitmap_{from,to}_arr32.
> 
> ...
> 
> > +	bitmap_copy_clear_tail((unsigned long *) (bitmap),	\
> > +			(const unsigned long *) (buf), (nbits))
> 
> Drop spaces after castings. Besides that it might be placed on a single line.
> 
> ...

OK
 
> 
> > +	bitmap_copy_clear_tail((unsigned long *) (buf),		\
> > +			(const unsigned long *) (bitmap), (nbits))
> 
> Ditto.
> 
> ...
> 
> > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
> > +{
> > +	const unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
> > +
> > +	while (bitmap < end) {
> > +		*buf = *bitmap++;
> > +		if (bitmap < end)
> > +			*buf |= (u64)(*bitmap++) << 32;
> > +		buf++;
> > +	}
> >  
> > +	/* Clear tail bits in last element of array beyond nbits. */
> > +	if (nbits % 64)
> > +		buf[-1] &= GENMASK_ULL(nbits, 0);
> 
> Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since
> end == bitmap? Or did I miss something?

BITS_TO_LONGS(0) == 0
BITS_TO_LONGS(1..32) == 1
BITS_TO_LONGS(33..64) == 2

The only potential problem with buf[-1] is nbits == 0, but fortunately
(0 % 64) == 0, and it doesn't happen.

Thanks,
Yury

> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH 2/5] lib: add bitmap_{from,to}_arr64

[Yury Norov]

On Fri, Apr 29, 2022 at 08:45:35AM -0700, Yury Norov wrote:
> On Fri, Apr 29, 2022 at 03:59:25PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote:
> > > Manipulating 64-bit arrays with bitmap functions is potentially dangerous
> > > because on 32-bit BE machines the order of halfwords doesn't match.
> > > Another issue is that compiler may throw a warning about out-of-boundary
> > > access.
> > > 
> > > This patch adds bitmap_{from,to}_arr64 functions in addition to existing
> > > bitmap_{from,to}_arr32.
> > 
> > ...
> > 
> > > +	bitmap_copy_clear_tail((unsigned long *) (bitmap),	\
> > > +			(const unsigned long *) (buf), (nbits))
> > 
> > Drop spaces after castings. Besides that it might be placed on a single line.
> > 
> > ...
> 
> OK
>  
> > 
> > > +	bitmap_copy_clear_tail((unsigned long *) (buf),		\
> > > +			(const unsigned long *) (bitmap), (nbits))
> > 
> > Ditto.
> > 
> > ...
> > 
> > > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
> > > +{
> > > +	const unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
> > > +
> > > +	while (bitmap < end) {
> > > +		*buf = *bitmap++;
> > > +		if (bitmap < end)
> > > +			*buf |= (u64)(*bitmap++) << 32;
> > > +		buf++;
> > > +	}
> > >  
> > > +	/* Clear tail bits in last element of array beyond nbits. */
> > > +	if (nbits % 64)
> > > +		buf[-1] &= GENMASK_ULL(nbits, 0);
> > 
> > Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since
> > end == bitmap? Or did I miss something?
> 
> BITS_TO_LONGS(0) == 0
> BITS_TO_LONGS(1..32) == 1
> BITS_TO_LONGS(33..64) == 2
> 
> The only potential problem with buf[-1] is nbits == 0, but fortunately
> (0 % 64) == 0, and it doesn't happen.
> 
> Thanks,
> Yury

Are there any other concerns? If no, I'll fix formatting and append it to
bitmap-for-next.

Thanks,
Yury

Re: [PATCH 2/5] lib: add bitmap_{from,to}_arr64

[Andy Shevchenko]

On Mon, May 02, 2022 at 01:06:56PM -0700, Yury Norov wrote:
> On Fri, Apr 29, 2022 at 08:45:35AM -0700, Yury Norov wrote:
> > On Fri, Apr 29, 2022 at 03:59:25PM +0300, Andy Shevchenko wrote:
> > > On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote:

...

> > > > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
> > > > +{
> > > > +	const unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
> > > > +
> > > > +	while (bitmap < end) {
> > > > +		*buf = *bitmap++;
> > > > +		if (bitmap < end)
> > > > +			*buf |= (u64)(*bitmap++) << 32;
> > > > +		buf++;
> > > > +	}
> > > >  
> > > > +	/* Clear tail bits in last element of array beyond nbits. */

in the last

> > > > +	if (nbits % 64)
> > > > +		buf[-1] &= GENMASK_ULL(nbits, 0);
> > > 
> > > Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since
> > > end == bitmap? Or did I miss something?
> > 
> > BITS_TO_LONGS(0) == 0
> > BITS_TO_LONGS(1..32) == 1
> > BITS_TO_LONGS(33..64) == 2
> > 
> > The only potential problem with buf[-1] is nbits == 0, but fortunately
> > (0 % 64) == 0, and it doesn't happen.

I see, perhaps adding a small comment would be nice to have to explain that -1
index is safe.

> Are there any other concerns? If no, I'll fix formatting and append it to
> bitmap-for-next.

Nope.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Guenter Roeck]

On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> Test newly added bitmap_{from,to}_arr64() functions similarly to
> already existing bitmap_{from,to}_arr32() tests.
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>

With this patch in linux-next (including next-20220519), I see lots of
bitmap test errors when booting 32-bit ppc images in qemu. Examples:

test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0", got "0,65"
...
test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128"
test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-129"
test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-130"
...
test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-209"
test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-210"

and so on. It only  gets worse from there, and ends with:

test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 4274
test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
', Time: 127267
test_bitmap: failed 337 out of 3801 tests

Other architectures and 64-bit ppc builds seem to be fine. 

Guenter

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

On Thu, May 19, 2022 at 8:09 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> > Test newly added bitmap_{from,to}_arr64() functions similarly to
> > already existing bitmap_{from,to}_arr32() tests.
> >
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
>
> With this patch in linux-next (including next-20220519), I see lots of
> bitmap test errors when booting 32-bit ppc images in qemu. Examples:
>
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0", got "0,65"
> ...
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128"
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-129"
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-130"
> ...
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-209"
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-210"
>
> and so on. It only  gets worse from there, and ends with:
>
> test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 4274
> test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
> ', Time: 127267
> test_bitmap: failed 337 out of 3801 tests
>
> Other architectures and 64-bit ppc builds seem to be fine.

Hi Guenter,

Thanks for letting me know. It's really weird because it has already
been for 2 weeks
in next with no issues. But I tested it on mips32, not powerpc. I'll
check what happens
there.

Can you please share your config and qemu image if possible?

Thanks,
Yury

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Guenter Roeck]

On 5/19/22 09:01, Yury Norov wrote:
> On Thu, May 19, 2022 at 8:09 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
>>> Test newly added bitmap_{from,to}_arr64() functions similarly to
>>> already existing bitmap_{from,to}_arr32() tests.
>>>
>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>
>> With this patch in linux-next (including next-20220519), I see lots of
>> bitmap test errors when booting 32-bit ppc images in qemu. Examples:
>>
>> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0", got "0,65"
>> ...
>> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128"
>> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-129"
>> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-130"
>> ...
>> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-209"
>> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-210"
>>
>> and so on. It only  gets worse from there, and ends with:
>>
>> test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 4274
>> test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
>> ', Time: 127267
>> test_bitmap: failed 337 out of 3801 tests
>>
>> Other architectures and 64-bit ppc builds seem to be fine.
> 
> Hi Guenter,
> 
> Thanks for letting me know. It's really weird because it has already
> been for 2 weeks
> in next with no issues. But I tested it on mips32, not powerpc. I'll
> check what happens
> there.
> 
Oh, I have seen the problem for a while, it is just that -next is in
such a bad shape that it is difficult to bisect individual problems.

> Can you please share your config and qemu image if possible?
> 

First, you have to revert commit b033767848c411
("powerpc/code-patching: Use jump_label for testing freed initmem")
to avoid a crash. After that, a recent version of qemu should work
with the following command line.

qemu-system-ppc -kernel arch/powerpc/boot/uImage -M mpc8544ds \
	-m 256 -no-reboot -initrd rootfs.cpio \
	--append "rdinit=/sbin/init coherent_pool=512k mem=256M console=ttyS0" \
	-monitor none -nographic

Configuration is mpc85xx_defconfig with CONFIG_TEST_BITMAP enabled.
I used the root file system (initrd) from
https://github.com/groeck/linux-build-test/blob/master/rootfs/ppc/rootfs.cpio.gz

Hope this helps,
Guenter

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

On Thu, May 19, 2022 at 11:04 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 5/19/22 09:01, Yury Norov wrote:
> > On Thu, May 19, 2022 at 8:09 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> >>> Test newly added bitmap_{from,to}_arr64() functions similarly to
> >>> already existing bitmap_{from,to}_arr32() tests.
> >>>
> >>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> >>
> >> With this patch in linux-next (including next-20220519), I see lots of
> >> bitmap test errors when booting 32-bit ppc images in qemu. Examples:
> >>
> >> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0", got "0,65"
> >> ...
> >> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128"
> >> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-129"
> >> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-130"
> >> ...
> >> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-209"
> >> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-210"
> >>
> >> and so on. It only  gets worse from there, and ends with:
> >>
> >> test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 4274
> >> test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
> >> ', Time: 127267
> >> test_bitmap: failed 337 out of 3801 tests
> >>
> >> Other architectures and 64-bit ppc builds seem to be fine.
> >
> > Hi Guenter,
> >
> > Thanks for letting me know. It's really weird because it has already
> > been for 2 weeks
> > in next with no issues. But I tested it on mips32, not powerpc. I'll
> > check what happens
> > there.
> >
> Oh, I have seen the problem for a while, it is just that -next is in
> such a bad shape that it is difficult to bisect individual problems.
>
> > Can you please share your config and qemu image if possible?
> >
>
> First, you have to revert commit b033767848c411
> ("powerpc/code-patching: Use jump_label for testing freed initmem")
> to avoid a crash. After that, a recent version of qemu should work
> with the following command line.
>
> qemu-system-ppc -kernel arch/powerpc/boot/uImage -M mpc8544ds \
>         -m 256 -no-reboot -initrd rootfs.cpio \
>         --append "rdinit=/sbin/init coherent_pool=512k mem=256M console=ttyS0" \
>         -monitor none -nographic
>
> Configuration is mpc85xx_defconfig with CONFIG_TEST_BITMAP enabled.
> I used the root file system (initrd) from
> https://github.com/groeck/linux-build-test/blob/master/rootfs/ppc/rootfs.cpio.gz

Yes, that helped a lot. Thanks, I was able to reproduce it. I'll take
a look shortly.

Thanks,
Yury

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

On Thu, May 19, 2022 at 08:09:29AM -0700, Guenter Roeck wrote:
> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> > Test newly added bitmap_{from,to}_arr64() functions similarly to
> > already existing bitmap_{from,to}_arr32() tests.
> > 
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> 
> With this patch in linux-next (including next-20220519), I see lots of
> bitmap test errors when booting 32-bit ppc images in qemu. Examples:
> 
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0", got "0,65"
> ...
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128"
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-129"
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65", got "0,65,128-130"
> ...
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-209"
> test_bitmap: [lib/test_bitmap.c:600] bitmaps contents differ: expected "0,65,128-143", got "0,65,128-143,208-210"
> 
> and so on. It only  gets worse from there, and ends with:
> 
> test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 4274
> test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
> ', Time: 127267
> test_bitmap: failed 337 out of 3801 tests
> 
> Other architectures and 64-bit ppc builds seem to be fine. 

So, the problem is in previous patch. I'll fold-in the following fix
into that if no objections.

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 52b9912a71ea..97c14845f452 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1585,7 +1585,7 @@ void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)

        /* Clear tail bits in the last element of array beyond nbits. */
        if (nbits % 64)
-               buf[-1] &= GENMASK_ULL(nbits, 0);
+               buf[-1] &= GENMASK_ULL(nbits % 64, 0);
 }
 EXPORT_SYMBOL(bitmap_to_arr64);
 #endif

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Guenter Roeck]

Hi,

On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> Test newly added bitmap_{from,to}_arr64() functions similarly to
> already existing bitmap_{from,to}_arr32() tests.
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>

Ever since this test is in the tree, several of my boot tests show
lots of messages such as

test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
...
test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)

but then:

test_bitmap: all 6550 tests passed

The message suggests an error, given that it is displayed with pr_err,
but the summary suggests otherwise.

Is the message just noise, or is there a problem ?

Thanks,
Guenter

> ---
>  lib/test_bitmap.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 0c82f07f74fc..d5923a640457 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -585,6 +585,30 @@ static void __init test_bitmap_arr32(void)
>  	}
>  }
>  
> +static void __init test_bitmap_arr64(void)
> +{
> +	unsigned int nbits, next_bit;
> +	u64 arr[EXP1_IN_BITS / 64];
> +	DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
> +
> +	memset(arr, 0xa5, sizeof(arr));
> +
> +	for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
> +		memset(bmap2, 0xff, sizeof(arr));
> +		bitmap_to_arr64(arr, exp1, nbits);
> +		bitmap_from_arr64(bmap2, arr, nbits);
> +		expect_eq_bitmap(bmap2, exp1, nbits);
> +
> +		next_bit = find_next_bit(bmap2, round_up(nbits, BITS_PER_LONG), nbits);
> +		if (next_bit < round_up(nbits, BITS_PER_LONG))
> +			pr_err("bitmap_copy_arr64(nbits == %d:"
> +				" tail is not safely cleared: %d\n", nbits, next_bit);
> +
> +		if (nbits < EXP1_IN_BITS - 64)
> +			expect_eq_uint(arr[DIV_ROUND_UP(nbits, 64)], 0xa5a5a5a5);
> +	}
> +}
> +
>  static void noinline __init test_mem_optimisations(void)
>  {
>  	DECLARE_BITMAP(bmap1, 1024);
> @@ -852,6 +876,7 @@ static void __init selftest(void)
>  	test_copy();
>  	test_replace();
>  	test_bitmap_arr32();
> +	test_bitmap_arr64();
>  	test_bitmap_parse();
>  	test_bitmap_parselist();
>  	test_bitmap_printlist();
> -- 
> 2.32.0
> 

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> > Test newly added bitmap_{from,to}_arr64() functions similarly to
> > already existing bitmap_{from,to}_arr32() tests.
> > 
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> 
> Ever since this test is in the tree, several of my boot tests show
> lots of messages such as
> 
> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
> test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
> ...
> test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
> test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)

This may be a real problem. Can you share what's the system is? What's
endianness and register length?

+ Alexander Lobakin, the author of the exact subtest.
 
> but then:
> 
> test_bitmap: all 6550 tests passed

It's because corresponding error path doesn't increment failed_tests
counter. I'll send a fix shortly.

> 
> The message suggests an error, given that it is displayed with pr_err,
> but the summary suggests otherwise.
> 
> Is the message just noise, or is there a problem ?
> 
> Thanks,
> Guenter
> 
> > ---
> >  lib/test_bitmap.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> > index 0c82f07f74fc..d5923a640457 100644
> > --- a/lib/test_bitmap.c
> > +++ b/lib/test_bitmap.c
> > @@ -585,6 +585,30 @@ static void __init test_bitmap_arr32(void)
> >  	}
> >  }
> >  
> > +static void __init test_bitmap_arr64(void)
> > +{
> > +	unsigned int nbits, next_bit;
> > +	u64 arr[EXP1_IN_BITS / 64];
> > +	DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
> > +
> > +	memset(arr, 0xa5, sizeof(arr));
> > +
> > +	for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
> > +		memset(bmap2, 0xff, sizeof(arr));
> > +		bitmap_to_arr64(arr, exp1, nbits);
> > +		bitmap_from_arr64(bmap2, arr, nbits);
> > +		expect_eq_bitmap(bmap2, exp1, nbits);
> > +
> > +		next_bit = find_next_bit(bmap2, round_up(nbits, BITS_PER_LONG), nbits);
> > +		if (next_bit < round_up(nbits, BITS_PER_LONG))
> > +			pr_err("bitmap_copy_arr64(nbits == %d:"
> > +				" tail is not safely cleared: %d\n", nbits, next_bit);
> > +
> > +		if (nbits < EXP1_IN_BITS - 64)
> > +			expect_eq_uint(arr[DIV_ROUND_UP(nbits, 64)], 0xa5a5a5a5);
> > +	}
> > +}
> > +
> >  static void noinline __init test_mem_optimisations(void)
> >  {
> >  	DECLARE_BITMAP(bmap1, 1024);
> > @@ -852,6 +876,7 @@ static void __init selftest(void)
> >  	test_copy();
> >  	test_replace();
> >  	test_bitmap_arr32();
> > +	test_bitmap_arr64();
> >  	test_bitmap_parse();
> >  	test_bitmap_parselist();
> >  	test_bitmap_printlist();
> > -- 
> > 2.32.0
> > 

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

On Sat, Feb 25, 2023 at 04:05:02PM -0800, Yury Norov wrote:
> On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
> > Hi,
> > 
> > On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> > > Test newly added bitmap_{from,to}_arr64() functions similarly to
> > > already existing bitmap_{from,to}_arr32() tests.
> > > 
> > > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > 
> > Ever since this test is in the tree, several of my boot tests show
> > lots of messages such as
> > 
> > test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> > test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
> > test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
> > ...
> > test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
> > test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)
> 
> This may be a real problem. Can you share what's the system is? What's
> endianness and register length?
> 
> + Alexander Lobakin, the author of the exact subtest.

Forgot to add
  
> > but then:
> > 
> > test_bitmap: all 6550 tests passed
> 
> It's because corresponding error path doesn't increment failed_tests
> counter. I'll send a fix shortly.
> 
> > 
> > The message suggests an error, given that it is displayed with pr_err,
> > but the summary suggests otherwise.
> > 
> > Is the message just noise, or is there a problem ?
> > 
> > Thanks,
> > Guenter
> > 
> > > ---
> > >  lib/test_bitmap.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> > > index 0c82f07f74fc..d5923a640457 100644
> > > --- a/lib/test_bitmap.c
> > > +++ b/lib/test_bitmap.c
> > > @@ -585,6 +585,30 @@ static void __init test_bitmap_arr32(void)
> > >  	}
> > >  }
> > >  
> > > +static void __init test_bitmap_arr64(void)
> > > +{
> > > +	unsigned int nbits, next_bit;
> > > +	u64 arr[EXP1_IN_BITS / 64];
> > > +	DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
> > > +
> > > +	memset(arr, 0xa5, sizeof(arr));
> > > +
> > > +	for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
> > > +		memset(bmap2, 0xff, sizeof(arr));
> > > +		bitmap_to_arr64(arr, exp1, nbits);
> > > +		bitmap_from_arr64(bmap2, arr, nbits);
> > > +		expect_eq_bitmap(bmap2, exp1, nbits);
> > > +
> > > +		next_bit = find_next_bit(bmap2, round_up(nbits, BITS_PER_LONG), nbits);
> > > +		if (next_bit < round_up(nbits, BITS_PER_LONG))
> > > +			pr_err("bitmap_copy_arr64(nbits == %d:"
> > > +				" tail is not safely cleared: %d\n", nbits, next_bit);
> > > +
> > > +		if (nbits < EXP1_IN_BITS - 64)
> > > +			expect_eq_uint(arr[DIV_ROUND_UP(nbits, 64)], 0xa5a5a5a5);
> > > +	}
> > > +}
> > > +
> > >  static void noinline __init test_mem_optimisations(void)
> > >  {
> > >  	DECLARE_BITMAP(bmap1, 1024);
> > > @@ -852,6 +876,7 @@ static void __init selftest(void)
> > >  	test_copy();
> > >  	test_replace();
> > >  	test_bitmap_arr32();
> > > +	test_bitmap_arr64();
> > >  	test_bitmap_parse();
> > >  	test_bitmap_parselist();
> > >  	test_bitmap_printlist();
> > > -- 
> > > 2.32.0
> > > 

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Guenter Roeck]

On 2/25/23 16:04, Yury Norov wrote:
> On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
>> Hi,
>>
>> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
>>> Test newly added bitmap_{from,to}_arr64() functions similarly to
>>> already existing bitmap_{from,to}_arr32() tests.
>>>
>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>
>> Ever since this test is in the tree, several of my boot tests show
>> lots of messages such as
>>
>> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
>> test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
>> test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
>> ...
>> test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
>> test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)
> 
> This may be a real problem. Can you share what's the system is? What's
> endianness and register length?
> 

https://kerneltests.org/builders/qemu-arm-v7-master/builds/532/steps/qemubuildcommand/logs/stdio
https://kerneltests.org/builders/qemu-arm-next/builds/2086/steps/qemubuildcommand/logs/stdio

are examples from different arm machines.

https://kerneltests.org/builders/qemu-xtensa-master/builds/2178/steps/qemubuildcommand/logs/stdio

is an example from xtensa.

https://kerneltests.org/builders/qemu-x86-master/builds/2248/steps/qemubuildcommand/logs/stdio

is an example from a 32-bit x86 test.

I am sure there are others; I only notice the messages if there is some other problem,
and came to believe that they must be noise because it happens across several
architectures and because it claimed at the end that all tests passed.

Just to confirm, I ran a test with big and little endian mips machines.
The problem is only seen with the little endian 32-bit machine. It is not
seen with the 32-bit big endian machine, and it is not seen with any of
the 64-bit machines (big and little endian).

Guenter

> + Alexander Lobakin, the author of the exact subtest.
>   
>> but then:
>>
>> test_bitmap: all 6550 tests passed
> 
> It's because corresponding error path doesn't increment failed_tests
> counter. I'll send a fix shortly.
> 
>>
>> The message suggests an error, given that it is displayed with pr_err,
>> but the summary suggests otherwise.
>>
>> Is the message just noise, or is there a problem ?
>>
>> Thanks,
>> Guenter
>>
>>> ---
>>>   lib/test_bitmap.c | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
>>> index 0c82f07f74fc..d5923a640457 100644
>>> --- a/lib/test_bitmap.c
>>> +++ b/lib/test_bitmap.c
>>> @@ -585,6 +585,30 @@ static void __init test_bitmap_arr32(void)
>>>   	}
>>>   }
>>>   
>>> +static void __init test_bitmap_arr64(void)
>>> +{
>>> +	unsigned int nbits, next_bit;
>>> +	u64 arr[EXP1_IN_BITS / 64];
>>> +	DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
>>> +
>>> +	memset(arr, 0xa5, sizeof(arr));
>>> +
>>> +	for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
>>> +		memset(bmap2, 0xff, sizeof(arr));
>>> +		bitmap_to_arr64(arr, exp1, nbits);
>>> +		bitmap_from_arr64(bmap2, arr, nbits);
>>> +		expect_eq_bitmap(bmap2, exp1, nbits);
>>> +
>>> +		next_bit = find_next_bit(bmap2, round_up(nbits, BITS_PER_LONG), nbits);
>>> +		if (next_bit < round_up(nbits, BITS_PER_LONG))
>>> +			pr_err("bitmap_copy_arr64(nbits == %d:"
>>> +				" tail is not safely cleared: %d\n", nbits, next_bit);
>>> +
>>> +		if (nbits < EXP1_IN_BITS - 64)
>>> +			expect_eq_uint(arr[DIV_ROUND_UP(nbits, 64)], 0xa5a5a5a5);
>>> +	}
>>> +}
>>> +
>>>   static void noinline __init test_mem_optimisations(void)
>>>   {
>>>   	DECLARE_BITMAP(bmap1, 1024);
>>> @@ -852,6 +876,7 @@ static void __init selftest(void)
>>>   	test_copy();
>>>   	test_replace();
>>>   	test_bitmap_arr32();
>>> +	test_bitmap_arr64();
>>>   	test_bitmap_parse();
>>>   	test_bitmap_parselist();
>>>   	test_bitmap_printlist();
>>> -- 
>>> 2.32.0
>>>

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Alexander Lobakin]

From: Yury Norov <yury.norov@gmail.com>
Date: Sat, 25 Feb 2023 16:06:45 -0800

> On Sat, Feb 25, 2023 at 04:05:02PM -0800, Yury Norov wrote:
>> On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
>>>> Test newly added bitmap_{from,to}_arr64() functions similarly to
>>>> already existing bitmap_{from,to}_arr32() tests.
>>>>
>>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>>
>>> Ever since this test is in the tree, several of my boot tests show
>>> lots of messages such as
>>>
>>> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)

Hmmm, the whole 4 bytes weren't touched.

>>> test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
>>> test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)

This is where it gets worse...

>>> ...
>>> test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
>>> test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)

I don't see the pattern how the actual result gets generated. But the
problem is in the bitmap code rather than in the subtest -- "must be"s
are fully correct.

Given that the 0xa5s are present in the upper 32 bits, it is Big Endian
I guess? Maybe even 32-bit Big Endian? Otherwise I'd start concerning
how comes it doesn't reproduce on x86_64s :D

>>
>> This may be a real problem. Can you share what's the system is? What's
>> endianness and register length?
>>
>> + Alexander Lobakin, the author of the exact subtest.
> 
> Forgot to add

Oh, thanks for letting me know!

>   
>>> but then:
>>>
>>> test_bitmap: all 6550 tests passed
>>
>> It's because corresponding error path doesn't increment failed_tests
>> counter. I'll send a fix shortly.

[...]

Thanks,
Olek

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Guenter Roeck]

On 2/27/23 06:46, Alexander Lobakin wrote:
> From: Yury Norov <yury.norov@gmail.com>
> Date: Sat, 25 Feb 2023 16:06:45 -0800
> 
>> On Sat, Feb 25, 2023 at 04:05:02PM -0800, Yury Norov wrote:
>>> On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
>>>>> Test newly added bitmap_{from,to}_arr64() functions similarly to
>>>>> already existing bitmap_{from,to}_arr32() tests.
>>>>>
>>>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>>>
>>>> Ever since this test is in the tree, several of my boot tests show
>>>> lots of messages such as
>>>>
>>>> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> 
> Hmmm, the whole 4 bytes weren't touched.
> 
>>>> test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
>>>> test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
> 
> This is where it gets worse...
> 
>>>> ...
>>>> test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
>>>> test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)
> 
> I don't see the pattern how the actual result gets generated. But the
> problem is in the bitmap code rather than in the subtest -- "must be"s
> are fully correct.
> 
> Given that the 0xa5s are present in the upper 32 bits, it is Big Endian
> I guess? Maybe even 32-bit Big Endian? Otherwise I'd start concerning
> how comes it doesn't reproduce on x86_64s :D
> 

It does reproduce on 32-bit x86 builds, and as far as I can see
it is only seen with 32-bit little endian systems.

Guenter

>>>
>>> This may be a real problem. Can you share what's the system is? What's
>>> endianness and register length?
>>>
>>> + Alexander Lobakin, the author of the exact subtest.
>>
>> Forgot to add
> 
> Oh, thanks for letting me know!
> 
>>    
>>>> but then:
>>>>
>>>> test_bitmap: all 6550 tests passed
>>>
>>> It's because corresponding error path doesn't increment failed_tests
>>> counter. I'll send a fix shortly.
> 
> [...]
> 
> Thanks,
> Olek

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

On Mon, Feb 27, 2023 at 06:59:12AM -0800, Guenter Roeck wrote:
> On 2/27/23 06:46, Alexander Lobakin wrote:
> > From: Yury Norov <yury.norov@gmail.com>
> > Date: Sat, 25 Feb 2023 16:06:45 -0800
> > 
> > > On Sat, Feb 25, 2023 at 04:05:02PM -0800, Yury Norov wrote:
> > > > On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
> > > > > Hi,
> > > > > 
> > > > > On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> > > > > > Test newly added bitmap_{from,to}_arr64() functions similarly to
> > > > > > already existing bitmap_{from,to}_arr32() tests.
> > > > > > 
> > > > > > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > > > > 
> > > > > Ever since this test is in the tree, several of my boot tests show
> > > > > lots of messages such as
> > > > > 
> > > > > test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> > 
> > Hmmm, the whole 4 bytes weren't touched.
> > 
> > > > > test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
> > > > > test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
> > 
> > This is where it gets worse...
> > 
> > > > > ...
> > > > > test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
> > > > > test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)
> > 
> > I don't see the pattern how the actual result gets generated. But the
> > problem is in the bitmap code rather than in the subtest -- "must be"s
> > are fully correct.
> > 
> > Given that the 0xa5s are present in the upper 32 bits, it is Big Endian
> > I guess? Maybe even 32-bit Big Endian? Otherwise I'd start concerning
> > how comes it doesn't reproduce on x86_64s :D
> > 
> 
> It does reproduce on 32-bit x86 builds, and as far as I can see
> it is only seen with 32-bit little endian systems.

Hi Guenter, Alexander,

I think that the reason for the failures like this:

> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)

is that bitmap_to_arr64 is overly optimized for 32-bit LE architectures.

Regarding this:

> test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)

I am not sure what happens, but because this again happens on 32-bit
LE only, I hope the following fix would help too.

Can you please check if the patch works for you? I don't have a 32-bit LE
machine in hand, and all my 32-bit VMs (arm and i386) refuse to load the
latest kernels for some weird reason, so it's only build-tested.

I'll give it a full-run when restore my 32-bit setups.

Thanks,
Yury

From 2881714db497aed103e310865da075e7b0ce7e1a Mon Sep 17 00:00:00 2001
From: Yury Norov <yury.norov@gmail.com>
Date: Mon, 27 Feb 2023 09:21:59 -0800
Subject: [PATCH] lib/bitmap: drop optimization of bitmap_{from,to}_arr64

bitmap_{from,to}_arr64() optimization is overly optimistic on 32-bit LE
architectures when it's wired to bitmap_copy_clear_tail().

bitmap_copy_clear_tail() takes care of unused bits in the bitmap up to
the next word boundary. But on 32-bit machines when copying bits from
bitmap to array of 64-bit words, it's expected that the unused part of
a recipient array must be cleared up to 64-bit boundary, so the last 4
bytes may stay untouched.

While the copying part of the optimization works correct, that clear-tail
trick makes corresponding tests reasonably fail when nbits % 64 <= 32:

test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)

Fix it by removing bitmap_{from,to}_arr64() optimization for 32-bit LE
arches.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Fixes: 0a97953fd2210 ("lib: add bitmap_{from,to}_arr64")
Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 8 +++-----
 lib/bitmap.c           | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 40e53a2ecc0d..5abc993903fb 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -302,12 +302,10 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
 #endif
 
 /*
- * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32
- * machines the order of hi and lo parts of numbers match the bitmap structure.
- * In both cases conversion is not needed when copying data from/to arrays of
- * u64.
+ * On 64-bit systems bitmaps are represented as u64 arrays internally. So,
+ * conversion is not needed when copying data from/to arrays of u64.
  */
-#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
+#if BITS_PER_LONG == 32
 void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits);
 void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits);
 #else
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 1c81413c51f8..ddb31015e38a 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1495,7 +1495,7 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
 EXPORT_SYMBOL(bitmap_to_arr32);
 #endif
 
-#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
+#if BITS_PER_LONG == 32
 /**
  * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap
  *	@bitmap: array of unsigned longs, the destination bitmap
-- 
2.34.1

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Guenter Roeck]

On 2/27/23 11:24, Yury Norov wrote:
> On Mon, Feb 27, 2023 at 06:59:12AM -0800, Guenter Roeck wrote:
>> On 2/27/23 06:46, Alexander Lobakin wrote:
>>> From: Yury Norov <yury.norov@gmail.com>
>>> Date: Sat, 25 Feb 2023 16:06:45 -0800
>>>
>>>> On Sat, Feb 25, 2023 at 04:05:02PM -0800, Yury Norov wrote:
>>>>> On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
>>>>>>> Test newly added bitmap_{from,to}_arr64() functions similarly to
>>>>>>> already existing bitmap_{from,to}_arr32() tests.
>>>>>>>
>>>>>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>>>>>
>>>>>> Ever since this test is in the tree, several of my boot tests show
>>>>>> lots of messages such as
>>>>>>
>>>>>> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
>>>
>>> Hmmm, the whole 4 bytes weren't touched.
>>>
>>>>>> test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
>>>>>> test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
>>>
>>> This is where it gets worse...
>>>
>>>>>> ...
>>>>>> test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
>>>>>> test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)
>>>
>>> I don't see the pattern how the actual result gets generated. But the
>>> problem is in the bitmap code rather than in the subtest -- "must be"s
>>> are fully correct.
>>>
>>> Given that the 0xa5s are present in the upper 32 bits, it is Big Endian
>>> I guess? Maybe even 32-bit Big Endian? Otherwise I'd start concerning
>>> how comes it doesn't reproduce on x86_64s :D
>>>
>>
>> It does reproduce on 32-bit x86 builds, and as far as I can see
>> it is only seen with 32-bit little endian systems.
> 
> Hi Guenter, Alexander,
> 
> I think that the reason for the failures like this:
> 
>> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> 
> is that bitmap_to_arr64 is overly optimized for 32-bit LE architectures.
> 
> Regarding this:
> 
>> test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
> 
> I am not sure what happens, but because this again happens on 32-bit
> LE only, I hope the following fix would help too.
> 
> Can you please check if the patch works for you? I don't have a 32-bit LE
> machine in hand, and all my 32-bit VMs (arm and i386) refuse to load the
> latest kernels for some weird reason, so it's only build-tested.
> 
> I'll give it a full-run when restore my 32-bit setups.
> 
> Thanks,
> Yury
> 
>>From 2881714db497aed103e310865da075e7b0ce7e1a Mon Sep 17 00:00:00 2001
> From: Yury Norov <yury.norov@gmail.com>
> Date: Mon, 27 Feb 2023 09:21:59 -0800
> Subject: [PATCH] lib/bitmap: drop optimization of bitmap_{from,to}_arr64
> 
> bitmap_{from,to}_arr64() optimization is overly optimistic on 32-bit LE
> architectures when it's wired to bitmap_copy_clear_tail().
> 
> bitmap_copy_clear_tail() takes care of unused bits in the bitmap up to
> the next word boundary. But on 32-bit machines when copying bits from
> bitmap to array of 64-bit words, it's expected that the unused part of
> a recipient array must be cleared up to 64-bit boundary, so the last 4
> bytes may stay untouched.
> 
> While the copying part of the optimization works correct, that clear-tail
> trick makes corresponding tests reasonably fail when nbits % 64 <= 32:
> 
> test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> 
> Fix it by removing bitmap_{from,to}_arr64() optimization for 32-bit LE
> arches.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 0a97953fd2210 ("lib: add bitmap_{from,to}_arr64")
> Signed-off-by: Yury Norov <yury.norov@gmail.com>

Tested with 32-bit i386 image. With this patch on top of
v6.2-12765-g982818426a0f, the log messages are gone. Without this patch,
they are still seen.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>   include/linux/bitmap.h | 8 +++-----
>   lib/bitmap.c           | 2 +-
>   2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 40e53a2ecc0d..5abc993903fb 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -302,12 +302,10 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
>   #endif
>   
>   /*
> - * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32
> - * machines the order of hi and lo parts of numbers match the bitmap structure.
> - * In both cases conversion is not needed when copying data from/to arrays of
> - * u64.
> + * On 64-bit systems bitmaps are represented as u64 arrays internally. So,
> + * conversion is not needed when copying data from/to arrays of u64.
>    */
> -#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
> +#if BITS_PER_LONG == 32
>   void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits);
>   void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits);
>   #else
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 1c81413c51f8..ddb31015e38a 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -1495,7 +1495,7 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
>   EXPORT_SYMBOL(bitmap_to_arr32);
>   #endif
>   
> -#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
> +#if BITS_PER_LONG == 32
>   /**
>    * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap
>    *	@bitmap: array of unsigned longs, the destination bitmap

Re: [PATCH 3/5] lib/bitmap: add test for bitmap_{from,to}_arr64

[Yury Norov]

On Mon, Feb 27, 2023 at 12:12:01PM -0800, Guenter Roeck wrote:
> On 2/27/23 11:24, Yury Norov wrote:
> > On Mon, Feb 27, 2023 at 06:59:12AM -0800, Guenter Roeck wrote:
> > > On 2/27/23 06:46, Alexander Lobakin wrote:
> > > > From: Yury Norov <yury.norov@gmail.com>
> > > > Date: Sat, 25 Feb 2023 16:06:45 -0800
> > > > 
> > > > > On Sat, Feb 25, 2023 at 04:05:02PM -0800, Yury Norov wrote:
> > > > > > On Sat, Feb 25, 2023 at 10:47:02AM -0800, Guenter Roeck wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Thu, Apr 28, 2022 at 01:51:14PM -0700, Yury Norov wrote:
> > > > > > > > Test newly added bitmap_{from,to}_arr64() functions similarly to
> > > > > > > > already existing bitmap_{from,to}_arr32() tests.
> > > > > > > > 
> > > > > > > > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > > > > > > 
> > > > > > > Ever since this test is in the tree, several of my boot tests show
> > > > > > > lots of messages such as
> > > > > > > 
> > > > > > > test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> > > > 
> > > > Hmmm, the whole 4 bytes weren't touched.
> > > > 
> > > > > > > test_bitmap: bitmap_to_arr64(nbits == 2): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000003)
> > > > > > > test_bitmap: bitmap_to_arr64(nbits == 3): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000007)
> > > > 
> > > > This is where it gets worse...
> > > > 
> > > > > > > ...
> > > > > > > test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
> > > > > > > test_bitmap: bitmap_to_arr64(nbits == 928): tail is not safely cleared: 0xa5a5a5a580000000 (must be 0x00000000ffffffff)
> > > > 
> > > > I don't see the pattern how the actual result gets generated. But the
> > > > problem is in the bitmap code rather than in the subtest -- "must be"s
> > > > are fully correct.
> > > > 
> > > > Given that the 0xa5s are present in the upper 32 bits, it is Big Endian
> > > > I guess? Maybe even 32-bit Big Endian? Otherwise I'd start concerning
> > > > how comes it doesn't reproduce on x86_64s :D
> > > > 
> > > 
> > > It does reproduce on 32-bit x86 builds, and as far as I can see
> > > it is only seen with 32-bit little endian systems.
> > 
> > Hi Guenter, Alexander,
> > 
> > I think that the reason for the failures like this:
> > 
> > > test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> > 
> > is that bitmap_to_arr64 is overly optimized for 32-bit LE architectures.
> > 
> > Regarding this:
> > 
> > > test_bitmap: bitmap_to_arr64(nbits == 927): tail is not safely cleared: 0xa5a5a5a500000000 (must be 0x000000007fffffff)
> > 
> > I am not sure what happens, but because this again happens on 32-bit
> > LE only, I hope the following fix would help too.
> > 
> > Can you please check if the patch works for you? I don't have a 32-bit LE
> > machine in hand, and all my 32-bit VMs (arm and i386) refuse to load the
> > latest kernels for some weird reason, so it's only build-tested.
> > 
> > I'll give it a full-run when restore my 32-bit setups.
> > 
> > Thanks,
> > Yury
> > 
> > > From 2881714db497aed103e310865da075e7b0ce7e1a Mon Sep 17 00:00:00 2001
> > From: Yury Norov <yury.norov@gmail.com>
> > Date: Mon, 27 Feb 2023 09:21:59 -0800
> > Subject: [PATCH] lib/bitmap: drop optimization of bitmap_{from,to}_arr64
> > 
> > bitmap_{from,to}_arr64() optimization is overly optimistic on 32-bit LE
> > architectures when it's wired to bitmap_copy_clear_tail().
> > 
> > bitmap_copy_clear_tail() takes care of unused bits in the bitmap up to
> > the next word boundary. But on 32-bit machines when copying bits from
> > bitmap to array of 64-bit words, it's expected that the unused part of
> > a recipient array must be cleared up to 64-bit boundary, so the last 4
> > bytes may stay untouched.
> > 
> > While the copying part of the optimization works correct, that clear-tail
> > trick makes corresponding tests reasonably fail when nbits % 64 <= 32:
> > 
> > test_bitmap: bitmap_to_arr64(nbits == 1): tail is not safely cleared: 0xa5a5a5a500000001 (must be 0x0000000000000001)
> > 
> > Fix it by removing bitmap_{from,to}_arr64() optimization for 32-bit LE
> > arches.
> > 
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Fixes: 0a97953fd2210 ("lib: add bitmap_{from,to}_arr64")
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> 
> Tested with 32-bit i386 image. With this patch on top of
> v6.2-12765-g982818426a0f, the log messages are gone. Without this patch,
> they are still seen.
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks!

Then, I'll submit it properly together with a fix for fail_counter.

Thanks,
Yury