bitmap: clean up and (micro?)optimization

[PATCH v1 1/2] bitmap: Use constants and macros from bits.h

Posted by Andy Shevchenko 2 years, 4 months ago

Use BITS_PER_LONG and BITS_PER_BYTE for BITMAP_MEM_ALIGNMENT.
Calculate bytes from bits for memcmp() and memset() with BITS_TO_BYTES().

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

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 1516ff979315..2d5042d1b501 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -354,9 +354,9 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr
 }
 
 #ifdef __LITTLE_ENDIAN
-#define BITMAP_MEM_ALIGNMENT 8
+#define BITMAP_MEM_ALIGNMENT	BITS_PER_BYTE
 #else
-#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long))
+#define BITMAP_MEM_ALIGNMENT	BITS_PER_LONG
 #endif
 #define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)
 
@@ -367,7 +367,7 @@ static inline bool bitmap_equal(const unsigned long *src1,
 		return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits));
 	if (__builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
 	    IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
-		return !memcmp(src1, src2, nbits / 8);
+		return !memcmp(src1, src2, BITS_TO_BYTES(nbits));
 	return __bitmap_equal(src1, src2, nbits);
 }
 
@@ -454,7 +454,7 @@ static __always_inline void bitmap_set(unsigned long *map, unsigned int start,
 		 IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) &&
 		 __builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
 		 IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
-		memset((char *)map + start / 8, 0xff, nbits / 8);
+		memset((char *)map + BITS_TO_BYTES(start), 0xff, BITS_TO_BYTES(nbits));
 	else
 		__bitmap_set(map, start, nbits);
 }
@@ -470,7 +470,7 @@ static __always_inline void bitmap_clear(unsigned long *map, unsigned int start,
 		 IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) &&
 		 __builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
 		 IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
-		memset((char *)map + start / 8, 0, nbits / 8);
+		memset((char *)map + BITS_TO_BYTES(start), 0x00, BITS_TO_BYTES(nbits));
 	else
 		__bitmap_clear(map, start, nbits);
 }
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/2] bitmap: Use constants and macros from bits.h

Posted by Rasmus Villemoes 2 years, 4 months ago

On 17/08/2023 18.54, Andy Shevchenko wrote:
> Use BITS_PER_LONG and BITS_PER_BYTE for BITMAP_MEM_ALIGNMENT.
> Calculate bytes from bits for memcmp() and memset() with BITS_TO_BYTES().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/bitmap.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 1516ff979315..2d5042d1b501 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -354,9 +354,9 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr
>  }
>  
>  #ifdef __LITTLE_ENDIAN
> -#define BITMAP_MEM_ALIGNMENT 8
> +#define BITMAP_MEM_ALIGNMENT	BITS_PER_BYTE
>  #else
> -#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long))
> +#define BITMAP_MEM_ALIGNMENT	BITS_PER_LONG
>  #endif
>  #define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)
>  
> @@ -367,7 +367,7 @@ static inline bool bitmap_equal(const unsigned long *src1,
>  		return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits));
>  	if (__builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
>  	    IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
> -		return !memcmp(src1, src2, nbits / 8);
> +		return !memcmp(src1, src2, BITS_TO_BYTES(nbits));

Please no. Currently, I can verify the arithmetic directly. Using such a
"helper" I'd have to know whether it just does /8 or if it's more like
the bitmap_words() thing which rounds up to a whole number of words. And
BITS_PER_BYTE (and similarly CHAR_BITS) really is, IMO, much less
readable than 8.

Rasmus

Re: [PATCH v1 1/2] bitmap: Use constants and macros from bits.h

Posted by Andy Shevchenko 2 years, 4 months ago

On Fri, Aug 18, 2023 at 08:28:21AM +0200, Rasmus Villemoes wrote:
> On 17/08/2023 18.54, Andy Shevchenko wrote:

> >  #ifdef __LITTLE_ENDIAN
> > -#define BITMAP_MEM_ALIGNMENT 8
> > +#define BITMAP_MEM_ALIGNMENT	BITS_PER_BYTE
> >  #else
> > -#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long))
> > +#define BITMAP_MEM_ALIGNMENT	BITS_PER_LONG
> >  #endif
> >  #define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)

What about this chunk? Does it worth to be updated?

...

> > -		return !memcmp(src1, src2, nbits / 8);
> > +		return !memcmp(src1, src2, BITS_TO_BYTES(nbits));
> 
> Please no. Currently, I can verify the arithmetic directly. Using such a
> "helper" I'd have to know whether it just does /8 or if it's more like
> the bitmap_words() thing which rounds up to a whole number of words. And
> BITS_PER_BYTE (and similarly CHAR_BITS) really is, IMO, much less
> readable than 8.

Okay, thank you for the comment!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] bitmap: Use constants and macros from bits.h

Posted by Rasmus Villemoes 2 years, 4 months ago

On 18/08/2023 10.50, Andy Shevchenko wrote:
> On Fri, Aug 18, 2023 at 08:28:21AM +0200, Rasmus Villemoes wrote:
>> On 17/08/2023 18.54, Andy Shevchenko wrote:
> 
>>>  #ifdef __LITTLE_ENDIAN
>>> -#define BITMAP_MEM_ALIGNMENT 8
>>> +#define BITMAP_MEM_ALIGNMENT	BITS_PER_BYTE
>>>  #else
>>> -#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long))
>>> +#define BITMAP_MEM_ALIGNMENT	BITS_PER_LONG
>>>  #endif
>>>  #define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)
> 
> What about this chunk? Does it worth to be updated?

IMHO, no, not in this way anyway. But the macros could perhaps use a
comment saying that they decide whether certain bitmap operations can be
turned into plain memxxx calls, which is why LE vs BE matters, and LE
just needs the bitmap to consist of a whole number of bytes while for BE
it must be a whole number of longs.

Rasmus