lib: Avoid redundant sizeof checking in __bitmap_weight() calculation.

PATCH In review Rakib Mullick 2 participants 3 messages Nov 14, 2017

Subsystems (2)

Library code Bitmap API

lore

Share & tools

lore

Rakib Mullick

author

2017-11-14 06:57 UTC #0 lore raw

PATCH 1/1 lib: Avoid redundant sizeof checking in __bitmap_weight() calculation.

Currently, during __bitmap_weight() calculation hweight_long() is used. Inside a hweight_long() a check has been made to figure out whether a hweight32() or hweight64() version to use.

However, it's unnecessary to do it in case of __bitmap_weight() calculation inside the loop. We can detect whether to use hweight32() or hweight64() upfront and call respective function directly. It also reduces the vmlinux size.

Before the patch:
text data bss dec hex filename
12901332 7798930 14541816 35242078 219c05e vmlinux

After the patch:
text data bss dec hex filename
12901331 7798930 14541816 35242077 219c05d vmlinux

Signed-off-by Rakib Mullick <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Yury Norov <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

---
Patch was created against torvald's tree (commit 43ff2f4db9d0f764).

 lib/bitmap.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

lib/bitmap.c +9 -4

index d8f0c09..552096f 100644
@@ -241,10 +241,15 @@ EXPORT_SYMBOL(__bitmap_subset);
 int __bitmap_weight(const unsigned long *bitmap, unsigned int bits)
 {
 	unsigned int k, lim = bits/BITS_PER_LONG;
-	int w = 0;
-
-	for (k = 0; k < lim; k++)
-		w += hweight_long(bitmap[k]);
+	int w = 0, is32 = sizeof(bitmap[0]) ? 1 : 0;
+
+	if (is32) {
+		for (k = 0; k < lim; k++)
+			w += hweight32(bitmap[k]);
+	} else {
+		for (k = 0; k < lim; k++)
+			w += hweight64(bitmap[k]);
+	}
 
 	if (bits % BITS_PER_LONG)
 		w += hweight_long(bitmap[k] & BITMAP_LAST_WORD_MASK(bits));

Signature · 1 line

2.9.3

Rasmus Villemoes

reviewer

2017-11-14 07:23 UTC #1 lore raw

Quoted 9 lines · Rakib Mullick

Currently, during __bitmap_weight() calculation hweight_long() is used. Inside a hweight_long() a check has been made to figure out whether a hweight32() or hweight64() version to use.
lib/bitmap.c +0 -0
index d8f0c09..552096f 100644

lib/bitmap.c EXPORT_SYMBOL(__bitmap_subset); · line 245

Quoted 9 lines

lib/bitmap.c +2 -4

@@ -241,10 +241,15 @@ EXPORT_SYMBOL(__bitmap_subset);
 int __bitmap_weight(const unsigned long *bitmap, unsigned int bits)
 {
        unsigned int k, lim = bits/BITS_PER_LONG;
-       int w = 0;
-
-       for (k = 0; k < lim; k++)
-               w += hweight_long(bitmap[k]);
+       int w = 0, is32 = sizeof(bitmap[0]) ? 1 : 0;
+

hint: sizeof() very rarely evaluates to zero... So this is the same as "is32 = 1". So the patch as-is is broken (and may explain the 1-byte delta in vmlinux). But even if this condition is fixed, the patch doesn't change anything, since the sizeof() evaluation is done at compile-time, regardless of whether it happens inside the inlined hweight_long or outside. So it is certainly not worth it to duplicate the loop.

Rasmus

Rakib Mullick

author

2017-11-14 08:59 UTC #2 lore raw

Quoted 9 lines · Rasmus Villemoes

hint: sizeof() very rarely evaluates to zero... So this is the same as "is32 = 1". So the patch as-is is broken (and may explain the 1-byte delta in vmlinux). But even if this condition is fixed, the patch doesn't change anything, since the sizeof() evaluation is done at compile-time, regardless of whether it happens inside the inlined hweight_long or outside. So it is certainly not worth it to duplicate the loop.

Right, no need to duplicate the loop. Checking the objdump output, it didn't changed anything at all. Fixed condition nullifies the vmlinux size advantage also. Drop it.

Thanks,
Rakib.