[PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Paul Mackerras]

Commit 2a98dc028f91 ("include/linux/bitmap.h: turn bitmap_set and
bitmap_clear into memset when possible", 2017-07-10) added an
optimization which effectively assumes that a contiguous set of
bits in a bitmap will be stored in a contiguous set of bytes of
the bitmap, in the case where the starting bit number and number of
bits are multiples of 8.  This is true for a little-endian
representation, but not for a big-endian representation, because we
number the bits of a long from right to left for both little-endian
and big-endian representations.

The optimization can still be done for big-endian platforms, but
only if the starting bit number and number of bits are multiples
of BITS_PER_LONG.  This adjusts the code to use 8 for little-endian
and BITS_PER_LONG for big-endian platforms.

Cc: stable@vger.kernel.org # v4.13
Fixes: 2a98dc028f91 ("include/linux/bitmap.h: turn bitmap_set and bitmap_clear into memset when possible")
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
This was found by inspection.

I'm pretty sure commit 2c6deb01525a is wrong as well on big-endian
machines.

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

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 700cf5f..44e2eb4 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -174,8 +174,10 @@ extern unsigned int bitmap_to_u32array(u32 *buf,
 				       unsigned int nbits);
 #ifdef __BIG_ENDIAN
 extern void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int nbits);
+#define BITMAP_MIN_UNIT	BITS_PER_LONG	/* have to access as longs */
 #else
 #define bitmap_copy_le bitmap_copy
+#define BITMAP_MIN_UNIT	8		/* can access as bytes if desired */
 #endif
 extern unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord, unsigned int nbits);
 extern int bitmap_print_to_pagebuf(bool list, char *buf,
@@ -317,8 +319,10 @@ static __always_inline void bitmap_set(unsigned long *map, unsigned int start,
 {
 	if (__builtin_constant_p(nbits) && nbits == 1)
 		__set_bit(start, map);
-	else if (__builtin_constant_p(start & 7) && IS_ALIGNED(start, 8) &&
-		 __builtin_constant_p(nbits & 7) && IS_ALIGNED(nbits, 8))
+	else if (__builtin_constant_p(start & (BITMAP_MIN_UNIT-1)) &&
+		 IS_ALIGNED(start, BITMAP_MIN_UNIT) &&
+		 __builtin_constant_p(nbits & (BITMAP_MIN_UNIT-1)) &&
+		 IS_ALIGNED(nbits, BITMAP_MIN_UNIT))
 		memset((char *)map + start / 8, 0xff, nbits / 8);
 	else
 		__bitmap_set(map, start, nbits);
@@ -329,8 +333,10 @@ static __always_inline void bitmap_clear(unsigned long *map, unsigned int start,
 {
 	if (__builtin_constant_p(nbits) && nbits == 1)
 		__clear_bit(start, map);
-	else if (__builtin_constant_p(start & 7) && IS_ALIGNED(start, 8) &&
-		 __builtin_constant_p(nbits & 7) && IS_ALIGNED(nbits, 8))
+	else if (__builtin_constant_p(start & (BITMAP_MIN_UNIT-1)) &&
+		 IS_ALIGNED(start, BITMAP_MIN_UNIT) &&
+		 __builtin_constant_p(nbits & (BITMAP_MIN_UNIT-1)) &&
+		 IS_ALIGNED(nbits, BITMAP_MIN_UNIT))
 		memset((char *)map + start / 8, 0, nbits / 8);
 	else
 		__bitmap_clear(map, start, nbits);
-- 
2.7.4

RE: [PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Matthew Wilcox]

SGFuZyBvbiwgZG9uJ3QgdGVsbCBtZSB5b3UgZm91bmQgdGhpcyBieSBpbnNwZWN0aW9uLiAgQXJl
IHlvdSBub3QgcnVubmluZyB0aGUgYml0bWFwIHRlc3RjYXNlLCBlbmFibGVkIGJ5IENPTkZJR19U
RVNUX0JJVE1BUD8gIEVpdGhlciB0aGF0IHNob3VsZCBiZSBwcm9kdWNpbmcgYW4gZXJyb3IsIG9y
IHRoZXJlJ3MgYSBtaXNzaW5nIHRlc3QgY2FzZSwgb3IgeW91ciBpbnNwZWN0aW9uIGlzIHdyb25n
IC4uLg0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFBhdWwgTWFja2Vy
cmFzIFttYWlsdG86cGF1bHVzQG96bGFicy5vcmddDQo+IFNlbnQ6IFdlZG5lc2RheSwgT2N0b2Jl
ciAyNSwgMjAxNyAyOjU3IEFNDQo+IFRvOiBNYXR0aGV3IFdpbGNveCA8bWF3aWxjb3hAbWljcm9z
b2Z0LmNvbT47IExpbnVzIFRvcnZhbGRzDQo+IDx0b3J2YWxkc0BsaW51eC1mb3VuZGF0aW9uLm9y
Zz47IGxrbWxAdmdlci5rZXJuZWwub3JnDQo+IENjOiBsaW51eHBwYy1kZXZAb3psYWJzLm9yZzsg
bGludXgtczM5MEB2Z2VyLmtlcm5lbC5vcmcNCj4gU3ViamVjdDogW1BBVENIXSBiaXRtYXA6IEZp
eCBvcHRpbWl6YXRpb24gb2YgYml0bWFwX3NldC9jbGVhciBmb3IgYmlnLWVuZGlhbg0KPiBtYWNo
aW5lcw0KPiANCj4gQ29tbWl0IDJhOThkYzAyOGY5MSAoImluY2x1ZGUvbGludXgvYml0bWFwLmg6
IHR1cm4gYml0bWFwX3NldCBhbmQNCj4gYml0bWFwX2NsZWFyIGludG8gbWVtc2V0IHdoZW4gcG9z
c2libGUiLCAyMDE3LTA3LTEwKSBhZGRlZCBhbg0KPiBvcHRpbWl6YXRpb24gd2hpY2ggZWZmZWN0
aXZlbHkgYXNzdW1lcyB0aGF0IGEgY29udGlndW91cyBzZXQgb2YNCj4gYml0cyBpbiBhIGJpdG1h
cCB3aWxsIGJlIHN0b3JlZCBpbiBhIGNvbnRpZ3VvdXMgc2V0IG9mIGJ5dGVzIG9mDQo+IHRoZSBi
aXRtYXAsIGluIHRoZSBjYXNlIHdoZXJlIHRoZSBzdGFydGluZyBiaXQgbnVtYmVyIGFuZCBudW1i
ZXIgb2YNCj4gYml0cyBhcmUgbXVsdGlwbGVzIG9mIDguICBUaGlzIGlzIHRydWUgZm9yIGEgbGl0
dGxlLWVuZGlhbg0KPiByZXByZXNlbnRhdGlvbiwgYnV0IG5vdCBmb3IgYSBiaWctZW5kaWFuIHJl
cHJlc2VudGF0aW9uLCBiZWNhdXNlIHdlDQo+IG51bWJlciB0aGUgYml0cyBvZiBhIGxvbmcgZnJv
bSByaWdodCB0byBsZWZ0IGZvciBib3RoIGxpdHRsZS1lbmRpYW4NCj4gYW5kIGJpZy1lbmRpYW4g
cmVwcmVzZW50YXRpb25zLg0KPiANCj4gVGhlIG9wdGltaXphdGlvbiBjYW4gc3RpbGwgYmUgZG9u
ZSBmb3IgYmlnLWVuZGlhbiBwbGF0Zm9ybXMsIGJ1dA0KPiBvbmx5IGlmIHRoZSBzdGFydGluZyBi
aXQgbnVtYmVyIGFuZCBudW1iZXIgb2YgYml0cyBhcmUgbXVsdGlwbGVzDQo+IG9mIEJJVFNfUEVS
X0xPTkcuICBUaGlzIGFkanVzdHMgdGhlIGNvZGUgdG8gdXNlIDggZm9yIGxpdHRsZS1lbmRpYW4N
Cj4gYW5kIEJJVFNfUEVSX0xPTkcgZm9yIGJpZy1lbmRpYW4gcGxhdGZvcm1zLg0KPiANCj4gQ2M6
IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcgIyB2NC4xMw0KPiBGaXhlczogMmE5OGRjMDI4ZjkxICgi
aW5jbHVkZS9saW51eC9iaXRtYXAuaDogdHVybiBiaXRtYXBfc2V0IGFuZA0KPiBiaXRtYXBfY2xl
YXIgaW50byBtZW1zZXQgd2hlbiBwb3NzaWJsZSIpDQo+IFNpZ25lZC1vZmYtYnk6IFBhdWwgTWFj
a2VycmFzIDxwYXVsdXNAb3psYWJzLm9yZz4NCj4gLS0tDQo+IFRoaXMgd2FzIGZvdW5kIGJ5IGlu
c3BlY3Rpb24uDQo+IA0KPiBJJ20gcHJldHR5IHN1cmUgY29tbWl0IDJjNmRlYjAxNTI1YSBpcyB3
cm9uZyBhcyB3ZWxsIG9uIGJpZy1lbmRpYW4NCj4gbWFjaGluZXMuDQo+IA0KPiAgaW5jbHVkZS9s
aW51eC9iaXRtYXAuaCB8IDE0ICsrKysrKysrKystLS0tDQo+ICAxIGZpbGUgY2hhbmdlZCwgMTAg
aW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9pbmNsdWRl
L2xpbnV4L2JpdG1hcC5oIGIvaW5jbHVkZS9saW51eC9iaXRtYXAuaA0KPiBpbmRleCA3MDBjZjVm
Li40NGUyZWI0IDEwMDY0NA0KPiAtLS0gYS9pbmNsdWRlL2xpbnV4L2JpdG1hcC5oDQo+ICsrKyBi
L2luY2x1ZGUvbGludXgvYml0bWFwLmgNCj4gQEAgLTE3NCw4ICsxNzQsMTAgQEAgZXh0ZXJuIHVu
c2lnbmVkIGludCBiaXRtYXBfdG9fdTMyYXJyYXkodTMyICpidWYsDQo+ICAJCQkJICAgICAgIHVu
c2lnbmVkIGludCBuYml0cyk7DQo+ICAjaWZkZWYgX19CSUdfRU5ESUFODQo+ICBleHRlcm4gdm9p
ZCBiaXRtYXBfY29weV9sZSh1bnNpZ25lZCBsb25nICpkc3QsIGNvbnN0IHVuc2lnbmVkIGxvbmcg
KnNyYywNCj4gdW5zaWduZWQgaW50IG5iaXRzKTsNCj4gKyNkZWZpbmUgQklUTUFQX01JTl9VTklU
CUJJVFNfUEVSX0xPTkcJLyogaGF2ZSB0byBhY2Nlc3MgYXMNCj4gbG9uZ3MgKi8NCj4gICNlbHNl
DQo+ICAjZGVmaW5lIGJpdG1hcF9jb3B5X2xlIGJpdG1hcF9jb3B5DQo+ICsjZGVmaW5lIEJJVE1B
UF9NSU5fVU5JVAk4CQkvKiBjYW4gYWNjZXNzIGFzIGJ5dGVzIGlmDQo+IGRlc2lyZWQgKi8NCj4g
ICNlbmRpZg0KPiAgZXh0ZXJuIHVuc2lnbmVkIGludCBiaXRtYXBfb3JkX3RvX3Bvcyhjb25zdCB1
bnNpZ25lZCBsb25nICpiaXRtYXAsDQo+IHVuc2lnbmVkIGludCBvcmQsIHVuc2lnbmVkIGludCBu
Yml0cyk7DQo+ICBleHRlcm4gaW50IGJpdG1hcF9wcmludF90b19wYWdlYnVmKGJvb2wgbGlzdCwg
Y2hhciAqYnVmLA0KPiBAQCAtMzE3LDggKzMxOSwxMCBAQCBzdGF0aWMgX19hbHdheXNfaW5saW5l
IHZvaWQgYml0bWFwX3NldCh1bnNpZ25lZCBsb25nDQo+ICptYXAsIHVuc2lnbmVkIGludCBzdGFy
dCwNCj4gIHsNCj4gIAlpZiAoX19idWlsdGluX2NvbnN0YW50X3AobmJpdHMpICYmIG5iaXRzID09
IDEpDQo+ICAJCV9fc2V0X2JpdChzdGFydCwgbWFwKTsNCj4gLQllbHNlIGlmIChfX2J1aWx0aW5f
Y29uc3RhbnRfcChzdGFydCAmIDcpICYmIElTX0FMSUdORUQoc3RhcnQsIDgpICYmDQo+IC0JCSBf
X2J1aWx0aW5fY29uc3RhbnRfcChuYml0cyAmIDcpICYmIElTX0FMSUdORUQobmJpdHMsIDgpKQ0K
PiArCWVsc2UgaWYgKF9fYnVpbHRpbl9jb25zdGFudF9wKHN0YXJ0ICYgKEJJVE1BUF9NSU5fVU5J
VC0xKSkgJiYNCj4gKwkJIElTX0FMSUdORUQoc3RhcnQsIEJJVE1BUF9NSU5fVU5JVCkgJiYNCj4g
KwkJIF9fYnVpbHRpbl9jb25zdGFudF9wKG5iaXRzICYgKEJJVE1BUF9NSU5fVU5JVC0xKSkgJiYN
Cj4gKwkJIElTX0FMSUdORUQobmJpdHMsIEJJVE1BUF9NSU5fVU5JVCkpDQo+ICAJCW1lbXNldCgo
Y2hhciAqKW1hcCArIHN0YXJ0IC8gOCwgMHhmZiwgbmJpdHMgLyA4KTsNCj4gIAllbHNlDQo+ICAJ
CV9fYml0bWFwX3NldChtYXAsIHN0YXJ0LCBuYml0cyk7DQo+IEBAIC0zMjksOCArMzMzLDEwIEBA
IHN0YXRpYyBfX2Fsd2F5c19pbmxpbmUgdm9pZCBiaXRtYXBfY2xlYXIodW5zaWduZWQNCj4gbG9u
ZyAqbWFwLCB1bnNpZ25lZCBpbnQgc3RhcnQsDQo+ICB7DQo+ICAJaWYgKF9fYnVpbHRpbl9jb25z
dGFudF9wKG5iaXRzKSAmJiBuYml0cyA9PSAxKQ0KPiAgCQlfX2NsZWFyX2JpdChzdGFydCwgbWFw
KTsNCj4gLQllbHNlIGlmIChfX2J1aWx0aW5fY29uc3RhbnRfcChzdGFydCAmIDcpICYmIElTX0FM
SUdORUQoc3RhcnQsIDgpICYmDQo+IC0JCSBfX2J1aWx0aW5fY29uc3RhbnRfcChuYml0cyAmIDcp
ICYmIElTX0FMSUdORUQobmJpdHMsIDgpKQ0KPiArCWVsc2UgaWYgKF9fYnVpbHRpbl9jb25zdGFu
dF9wKHN0YXJ0ICYgKEJJVE1BUF9NSU5fVU5JVC0xKSkgJiYNCj4gKwkJIElTX0FMSUdORUQoc3Rh
cnQsIEJJVE1BUF9NSU5fVU5JVCkgJiYNCj4gKwkJIF9fYnVpbHRpbl9jb25zdGFudF9wKG5iaXRz
ICYgKEJJVE1BUF9NSU5fVU5JVC0xKSkgJiYNCj4gKwkJIElTX0FMSUdORUQobmJpdHMsIEJJVE1B
UF9NSU5fVU5JVCkpDQo+ICAJCW1lbXNldCgoY2hhciAqKW1hcCArIHN0YXJ0IC8gOCwgMCwgbmJp
dHMgLyA4KTsNCj4gIAllbHNlDQo+ICAJCV9fYml0bWFwX2NsZWFyKG1hcCwgc3RhcnQsIG5iaXRz
KTsNCj4gLS0NCj4gMi43LjQNCg0K

Re: [PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Paul Mackerras]

On Wed, Oct 25, 2017 at 07:39:48AM +0000, Matthew Wilcox wrote:
> Hang on, don't tell me you found this by inspection.  Are you not running the bitmap testcase, enabled by CONFIG_TEST_BITMAP?  Either that should be producing an error, or there's a missing test case, or your inspection is wrong ...

I did find it by inspection.  I was looking for a version of the
bitmap_* API that does little-endian style bitmaps on all systems, and
the inline bitmap_set() does that in the case where it calls memset,
but not in the case where it calls __bitmap_set.

I'll fire up a big-endian system tomorrow when I get to work to run
the test case.  (PPC64 is almost entirely little-endian these days as
far as the IBM POWER systems are concerned.)

In any case, it's pretty clearly wrong as it is.  On a big-endian
64-bit system, bitmap_set(p, 56, 16) should set bytes 0 and 15 to
0xff, and there's no way a single memset can do that.

Paul.

(and yes, I stuffed up the address for lkml)

RE: [PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Matthew Wilcox]

From: Paul Mackerras [mailto:paulus@ozlabs.org]
> On Wed, Oct 25, 2017 at 07:39:48AM +0000, Matthew Wilcox wrote:
> > Hang on, don't tell me you found this by inspection.  Are you not running the
> bitmap testcase, enabled by CONFIG_TEST_BITMAP?  Either that should be
> producing an error, or there's a missing test case, or your inspection is wrong ...
> 
> I did find it by inspection.  I was looking for a version of the
> bitmap_* API that does little-endian style bitmaps on all systems, and
> the inline bitmap_set() does that in the case where it calls memset,
> but not in the case where it calls __bitmap_set.

I do believe that you noticed it by inspection, but you shouldn't've had to.  I thought we had a comprehensive set of tests for exactly this, which means that either 01.org isn't running the right set of tests on a BE system or the tests are broken.

> I'll fire up a big-endian system tomorrow when I get to work to run
> the test case.  (PPC64 is almost entirely little-endian these days as
> far as the IBM POWER systems are concerned.)
> 
> In any case, it's pretty clearly wrong as it is.  On a big-endian
> 64-bit system, bitmap_set(p, 56, 16) should set bytes 0 and 15 to
> 0xff, and there's no way a single memset can do that.

So ... this loop should include that case, right?

        for (start = 0; start < 1024; start += 8) {
                memset(bmap1, 0x5a, sizeof(bmap1));
                memset(bmap2, 0x5a, sizeof(bmap2));
                for (nbits = 0; nbits < 1024 - start; nbits += 8) {
                        bitmap_set(bmap1, start, nbits);
                        __bitmap_set(bmap2, start, nbits);
                        if (!bitmap_equal(bmap1, bmap2, 1024))
                                printk("set not equal %d %d\n", start, nbits);
                        if (!__bitmap_equal(bmap1, bmap2, 1024))
                                printk("set not __equal %d %d\n", start, nbits);

RE: [PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Matthew Wilcox]

RnJvbTogUGF1bCBNYWNrZXJyYXMgW21haWx0bzpwYXVsdXNAb3psYWJzLm9yZ10NCj4gT24gV2Vk
LCBPY3QgMjUsIDIwMTcgYXQgMDc6Mzk6NDhBTSArMDAwMCwgTWF0dGhldyBXaWxjb3ggd3JvdGU6
DQo+ID4gSGFuZyBvbiwgZG9uJ3QgdGVsbCBtZSB5b3UgZm91bmQgdGhpcyBieSBpbnNwZWN0aW9u
LiAgQXJlIHlvdSBub3QgcnVubmluZyB0aGUNCj4gYml0bWFwIHRlc3RjYXNlLCBlbmFibGVkIGJ5
IENPTkZJR19URVNUX0JJVE1BUD8gIEVpdGhlciB0aGF0IHNob3VsZCBiZQ0KPiBwcm9kdWNpbmcg
YW4gZXJyb3IsIG9yIHRoZXJlJ3MgYSBtaXNzaW5nIHRlc3QgY2FzZSwgb3IgeW91ciBpbnNwZWN0
aW9uIGlzIHdyb25nIC4uLg0KPiANCj4gSSBkaWQgZmluZCBpdCBieSBpbnNwZWN0aW9uLiAgSSB3
YXMgbG9va2luZyBmb3IgYSB2ZXJzaW9uIG9mIHRoZQ0KPiBiaXRtYXBfKiBBUEkgdGhhdCBkb2Vz
IGxpdHRsZS1lbmRpYW4gc3R5bGUgYml0bWFwcyBvbiBhbGwgc3lzdGVtcywgYW5kDQo+IHRoZSBp
bmxpbmUgYml0bWFwX3NldCgpIGRvZXMgdGhhdCBpbiB0aGUgY2FzZSB3aGVyZSBpdCBjYWxscyBt
ZW1zZXQsDQo+IGJ1dCBub3QgaW4gdGhlIGNhc2Ugd2hlcmUgaXQgY2FsbHMgX19iaXRtYXBfc2V0
Lg0KDQpJIGRvIGJlbGlldmUgdGhhdCB5b3Ugbm90aWNlZCBpdCBieSBpbnNwZWN0aW9uLCBidXQg
eW91IHNob3VsZG4ndCd2ZSBoYWQgdG8uICBJIHRob3VnaHQgd2UgaGFkIGEgY29tcHJlaGVuc2l2
ZSBzZXQgb2YgdGVzdHMgZm9yIGV4YWN0bHkgdGhpcywgd2hpY2ggbWVhbnMgdGhhdCBlaXRoZXIg
MDEub3JnIGlzbid0IHJ1bm5pbmcgdGhlIHJpZ2h0IHNldCBvZiB0ZXN0cyBvbiBhIEJFIHN5c3Rl
bSBvciB0aGUgdGVzdHMgYXJlIGJyb2tlbi4NCg0KPiBJJ2xsIGZpcmUgdXAgYSBiaWctZW5kaWFu
IHN5c3RlbSB0b21vcnJvdyB3aGVuIEkgZ2V0IHRvIHdvcmsgdG8gcnVuDQo+IHRoZSB0ZXN0IGNh
c2UuICAoUFBDNjQgaXMgYWxtb3N0IGVudGlyZWx5IGxpdHRsZS1lbmRpYW4gdGhlc2UgZGF5cyBh
cw0KPiBmYXIgYXMgdGhlIElCTSBQT1dFUiBzeXN0ZW1zIGFyZSBjb25jZXJuZWQuKQ0KPiANCj4g
SW4gYW55IGNhc2UsIGl0J3MgcHJldHR5IGNsZWFybHkgd3JvbmcgYXMgaXQgaXMuICBPbiBhIGJp
Zy1lbmRpYW4NCj4gNjQtYml0IHN5c3RlbSwgYml0bWFwX3NldChwLCA1NiwgMTYpIHNob3VsZCBz
ZXQgYnl0ZXMgMCBhbmQgMTUgdG8NCj4gMHhmZiwgYW5kIHRoZXJlJ3Mgbm8gd2F5IGEgc2luZ2xl
IG1lbXNldCBjYW4gZG8gdGhhdC4NCg0KU28gLi4uIHRoaXMgbG9vcCBzaG91bGQgaW5jbHVkZSB0
aGF0IGNhc2UsIHJpZ2h0Pw0KDQogICAgICAgIGZvciAoc3RhcnQgPSAwOyBzdGFydCA8IDEwMjQ7
IHN0YXJ0ICs9IDgpIHsNCiAgICAgICAgICAgICAgICBtZW1zZXQoYm1hcDEsIDB4NWEsIHNpemVv
ZihibWFwMSkpOw0KICAgICAgICAgICAgICAgIG1lbXNldChibWFwMiwgMHg1YSwgc2l6ZW9mKGJt
YXAyKSk7DQogICAgICAgICAgICAgICAgZm9yIChuYml0cyA9IDA7IG5iaXRzIDwgMTAyNCAtIHN0
YXJ0OyBuYml0cyArPSA4KSB7DQogICAgICAgICAgICAgICAgICAgICAgICBiaXRtYXBfc2V0KGJt
YXAxLCBzdGFydCwgbmJpdHMpOw0KICAgICAgICAgICAgICAgICAgICAgICAgX19iaXRtYXBfc2V0
KGJtYXAyLCBzdGFydCwgbmJpdHMpOw0KICAgICAgICAgICAgICAgICAgICAgICAgaWYgKCFiaXRt
YXBfZXF1YWwoYm1hcDEsIGJtYXAyLCAxMDI0KSkNCiAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgcHJpbnRrKCJzZXQgbm90IGVxdWFsICVkICVkXG4iLCBzdGFydCwgbmJpdHMpOw0KICAg
ICAgICAgICAgICAgICAgICAgICAgaWYgKCFfX2JpdG1hcF9lcXVhbChibWFwMSwgYm1hcDIsIDEw
MjQpKQ0KICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBwcmludGsoInNldCBub3QgX19l
cXVhbCAlZCAlZFxuIiwgc3RhcnQsIG5iaXRzKTsNCg0KDQo=

Re: [PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Michael Ellerman]

Paul Mackerras <paulus@ozlabs.org> writes:

> On Wed, Oct 25, 2017 at 07:39:48AM +0000, Matthew Wilcox wrote:
>> Hang on, don't tell me you found this by inspection. Are you not
>> running the bitmap testcase, enabled by CONFIG_TEST_BITMAP? Either
>> that should be producing an error, or there's a missing test case, or
>> your inspection is wrong ...

Oops.

  [    1.907533] test_bitmap: test 13: input is '0-2047:128/256' OK, Time: 1359
  [    1.907596] set not equal 0 8
  [    1.907640] set not __equal 0 8
  [    1.907684] clear not equal 0 8
  [    1.907728] clear not __equal 0 8
  [    1.907772] set not equal 0 16
  [    1.907816] set not __equal 0 16
  [    1.907861] clear not equal 0 16
  [    1.907905] clear not __equal 0 16
  [    1.907949] set not equal 0 24
  [    1.907993] set not __equal 0 24
  [    1.908038] clear not equal 0 24
  [    1.908082] clear not __equal 0 24
  ...
  [ snip ~30,000 lines ]
  ...
  [    3.345729] set not equal 1008 8
  [    3.345773] set not __equal 1008 8
  [    3.345818] clear not equal 1008 8
  [    3.345863] clear not __equal 1008 8

  [    3.345909] test_bitmap: all 460506 tests passed
                              ^^^^^^^^^^^^^^^^^^^^^^^
                              O_o

With the patch:

  [    1.904393] test_bitmap: test 13: input is '0-2047:128/256' OK, Time: 1359
  [    1.916270] test_bitmap: all 460506 tests passed

cheers

Re: [PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Michael Ellerman]

Matthew Wilcox <willy@infradead.org> writes:

> (I don't think I can reliably send patches from outlook; sorry for
> breaking the threading)
>
> I see where we're not incrementing the failure count ... try this patch!
>
> --- 8< ---
>
> Subject: Fix bitmap optimisation tests to report errors correctly
> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> I had neglected to increment the error counter when the tests failed,
> which made the tests noisy when they fail, but not actually return an
> error code.
>
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> Cc: stable@kernel.org

Thanks, that works for me.

  test_bitmap: failed 31840 out of 460506 tests

cheers

Re: [PATCH] bitmap: Fix optimization of bitmap_set/clear for big-endian machines

[Matthew Wilcox]

(I don't think I can reliably send patches from outlook; sorry for
breaking the threading)

I see where we're not incrementing the failure count ... try this patch!

--- 8< ---

Subject: Fix bitmap optimisation tests to report errors correctly
From: Matthew Wilcox <mawilcox@microsoft.com>

I had neglected to increment the error counter when the tests failed,
which made the tests noisy when they fail, but not actually return an
error code.

Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
Cc: stable@kernel.org

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index aa1f2669bdd5..ae8a830e4e54 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -430,23 +430,32 @@ static void noinline __init test_mem_optimisations(void)
 	unsigned int start, nbits;
 
 	for (start = 0; start < 1024; start += 8) {
-		memset(bmap1, 0x5a, sizeof(bmap1));
-		memset(bmap2, 0x5a, sizeof(bmap2));
 		for (nbits = 0; nbits < 1024 - start; nbits += 8) {
+			memset(bmap1, 0x5a, sizeof(bmap1));
+			memset(bmap2, 0x5a, sizeof(bmap2));
+
 			bitmap_set(bmap1, start, nbits);
 			__bitmap_set(bmap2, start, nbits);
-			if (!bitmap_equal(bmap1, bmap2, 1024))
+			if (!bitmap_equal(bmap1, bmap2, 1024)) {
 				printk("set not equal %d %d\n", start, nbits);
-			if (!__bitmap_equal(bmap1, bmap2, 1024))
+				failed_tests++;
+			}
+			if (!__bitmap_equal(bmap1, bmap2, 1024)) {
 				printk("set not __equal %d %d\n", start, nbits);
+				failed_tests++;
+			}
 
 			bitmap_clear(bmap1, start, nbits);
 			__bitmap_clear(bmap2, start, nbits);
-			if (!bitmap_equal(bmap1, bmap2, 1024))
+			if (!bitmap_equal(bmap1, bmap2, 1024)) {
 				printk("clear not equal %d %d\n", start, nbits);
-			if (!__bitmap_equal(bmap1, bmap2, 1024))
+				failed_tests++;
+			}
+			if (!__bitmap_equal(bmap1, bmap2, 1024)) {
 				printk("clear not __equal %d %d\n", start,
 									nbits);
+				failed_tests++;
+			}
 		}
 	}
 }