[PATCH v4] xen: simplify bitmap_to_xenctl_bitmap for little endian

[PATCH v4] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Stefano Stabellini]

The little endian implementation of bitmap_to_xenctl_bitmap leads to
unnecessary xmallocs and xfrees. Given that Xen only supports little
endian architectures, it is worth optimizing.

This patch removes the need for the xmalloc on little endian
architectures.

Remove clamp_last_byte as it is only called once and only needs to
modify one byte. Inline it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
Changes in v4:
- improve commit message
- remove clamp_last_byte
---
 xen/common/bitmap.c | 64 +++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index 3da63a32a6..10d19f4c0d 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -40,21 +40,6 @@
  * for the best explanations of this ordering.
  */
 
-/*
- * If a bitmap has a number of bits which is not a multiple of 8 then
- * the last few bits of the last byte of the bitmap can be
- * unexpectedly set which can confuse consumers (e.g. in the tools)
- * who also round up their loops to 8 bits. Ensure we clear those left
- * over bits so as to prevent surprises.
- */
-static void clamp_last_byte(uint8_t *bp, unsigned int nbits)
-{
-	unsigned int remainder = nbits % 8;
-
-	if (remainder)
-		bp[nbits/8] &= (1U << remainder) - 1;
-}
-
 int __bitmap_empty(const unsigned long *bitmap, unsigned int bits)
 {
 	int k, lim = bits/BITS_PER_LONG;
@@ -338,7 +323,6 @@ static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
 			nbits -= 8;
 		}
 	}
-	clamp_last_byte(bp, nbits);
 }
 
 static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
@@ -363,7 +347,6 @@ static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
 				unsigned int nbits)
 {
 	memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
-	clamp_last_byte(bp, nbits);
 }
 
 static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
@@ -377,6 +360,13 @@ static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
 
 #endif
 
+/*
+ * If a bitmap has a number of bits which is not a multiple of 8 then
+ * the last few bits of the last byte of the bitmap can be
+ * unexpectedly set which can confuse consumers (e.g. in the tools)
+ * who also round up their loops to 8 bits. Ensure we clear those left
+ * over bits so as to prevent surprises.
+ */
 int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
                             const unsigned long *bitmap, unsigned int nbits)
 {
@@ -384,21 +374,43 @@ int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
     uint8_t zero = 0;
     int err = 0;
     unsigned int xen_bytes = DIV_ROUND_UP(nbits, BITS_PER_BYTE);
-    uint8_t *bytemap = xmalloc_array(uint8_t, xen_bytes);
-
-    if ( !bytemap )
-        return -ENOMEM;
+    uint8_t last;
 
     guest_bytes = DIV_ROUND_UP(xenctl_bitmap->nr_bits, BITS_PER_BYTE);
     copy_bytes  = min(guest_bytes, xen_bytes);
 
-    bitmap_long_to_byte(bytemap, bitmap, nbits);
+    if ( IS_ENABLED(__BIG_ENDIAN) )
+    {
+        uint8_t *bytemap = xmalloc_array(uint8_t, xen_bytes);
 
-    if ( copy_bytes &&
-         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
-        err = -EFAULT;
+        if ( !bytemap )
+            return -ENOMEM;
 
-    xfree(bytemap);
+        bitmap_long_to_byte(bytemap, bitmap, nbits);
+        last = bytemap[nbits / 8];
+
+        if ( copy_bytes > 1 &&
+             copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes - 1) )
+            err = -EFAULT;
+
+        xfree(bytemap);
+    }
+    else
+    {
+        const uint8_t *bytemap = (const uint8_t *)bitmap;
+        last = bytemap[nbits / 8];
+
+        if ( copy_bytes > 1 &&
+             copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes - 1) )
+            err = -EFAULT;
+    }
+
+    if ( nbits % 8 )
+        last &= (1U << (nbits % 8)) - 1;
+
+    if ( copy_bytes > 0 &&
+         copy_to_guest_offset(xenctl_bitmap->bitmap, copy_bytes - 1, &last, 1) )
+        err = -EFAULT;
 
     for ( i = copy_bytes; !err && i < guest_bytes; i++ )
         if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )
-- 
2.25.1

[PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Jan Beulich]

From: Stefano Stabellini <stefano.stabellini@amd.com>

The little endian implementation of bitmap_to_xenctl_bitmap leads to
unnecessary xmallocs and xfrees. Given that Xen only supports little
endian architectures, it is worth optimizing.

This patch removes the need for the xmalloc on little endian
architectures.

Remove clamp_last_byte as it is only called once and only needs to
modify one byte. Inline it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Fix IS_ENABLED() use. Keep more code common. Move comment.
    Convert LE bitmap_long_to_byte() to just a declaration.

--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -40,21 +40,6 @@
  * for the best explanations of this ordering.
  */
 
-/*
- * If a bitmap has a number of bits which is not a multiple of 8 then
- * the last few bits of the last byte of the bitmap can be
- * unexpectedly set which can confuse consumers (e.g. in the tools)
- * who also round up their loops to 8 bits. Ensure we clear those left
- * over bits so as to prevent surprises.
- */
-static void clamp_last_byte(uint8_t *bp, unsigned int nbits)
-{
-	unsigned int remainder = nbits % 8;
-
-	if (remainder)
-		bp[nbits/8] &= (1U << remainder) - 1;
-}
-
 int __bitmap_empty(const unsigned long *bitmap, unsigned int bits)
 {
 	int k, lim = bits/BITS_PER_LONG;
@@ -338,7 +323,6 @@ static void bitmap_long_to_byte(uint8_t
 			nbits -= 8;
 		}
 	}
-	clamp_last_byte(bp, nbits);
 }
 
 static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
@@ -359,12 +343,11 @@ static void bitmap_byte_to_long(unsigned
 
 #elif defined(__LITTLE_ENDIAN)
 
-static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
-				unsigned int nbits)
-{
-	memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
-	clamp_last_byte(bp, nbits);
-}
+#define LITTLE_ENDIAN 1 /* For IS_ENABLED(). */
+
+/* Unused function, but a declaration is needed. */
+void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
+			 unsigned int nbits);
 
 static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
 				unsigned int nbits)
@@ -384,22 +367,46 @@ int bitmap_to_xenctl_bitmap(struct xenct
     uint8_t zero = 0;
     int err = 0;
     unsigned int xen_bytes = DIV_ROUND_UP(nbits, BITS_PER_BYTE);
-    uint8_t *bytemap = xmalloc_array(uint8_t, xen_bytes);
+    const uint8_t *bytemap;
+    uint8_t last, *buf = NULL;
 
-    if ( !bytemap )
-        return -ENOMEM;
+    if ( !IS_ENABLED(LITTLE_ENDIAN) )
+    {
+        buf = xmalloc_array(uint8_t, xen_bytes);
+        if ( !buf )
+            return -ENOMEM;
+
+        bitmap_long_to_byte(buf, bitmap, nbits);
+
+        bytemap = buf;
+    }
+    else
+        bytemap = (const uint8_t *)bitmap;
 
     guest_bytes = DIV_ROUND_UP(xenctl_bitmap->nr_bits, BITS_PER_BYTE);
     copy_bytes  = min(guest_bytes, xen_bytes);
 
-    bitmap_long_to_byte(bytemap, bitmap, nbits);
+    if ( copy_bytes > 1 &&
+         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes - 1) )
+        err = -EFAULT;
+
+    /*
+     * If a bitmap has a number of bits which is not a multiple of 8 then the
+     * last few bits of the last byte of the bitmap can be unexpectedly set,
+     * which can confuse consumers (e.g. in the tools), who also may round up
+     * their loops to 8 bits. Ensure we clear those left over bits so as to
+     * prevent surprises.
+     */
+    last = bytemap[nbits / 8];
+    if ( nbits % 8 )
+        last &= (1U << (nbits % 8)) - 1;
+
+    xfree(buf);
 
     if ( copy_bytes &&
-         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
+         copy_to_guest_offset(xenctl_bitmap->bitmap, copy_bytes - 1, &last, 1) )
         err = -EFAULT;
 
-    xfree(bytemap);
-
     for ( i = copy_bytes; !err && i < guest_bytes; i++ )
         if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )
             err = -EFAULT;

Re: [PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Stefano Stabellini]

On Tue, 1 Apr 2025, Jan Beulich wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> The little endian implementation of bitmap_to_xenctl_bitmap leads to
> unnecessary xmallocs and xfrees. Given that Xen only supports little
> endian architectures, it is worth optimizing.
> 
> This patch removes the need for the xmalloc on little endian
> architectures.
> 
> Remove clamp_last_byte as it is only called once and only needs to
> modify one byte. Inline it.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v5: Fix IS_ENABLED() use. Keep more code common. Move comment.
>     Convert LE bitmap_long_to_byte() to just a declaration.

Thanks Jan, I looked at your version carefully and it looks correct to
me. I could give my reviewed-by but it looks weird given that I am also
the first author.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -40,21 +40,6 @@
>   * for the best explanations of this ordering.
>   */
>  
> -/*
> - * If a bitmap has a number of bits which is not a multiple of 8 then
> - * the last few bits of the last byte of the bitmap can be
> - * unexpectedly set which can confuse consumers (e.g. in the tools)
> - * who also round up their loops to 8 bits. Ensure we clear those left
> - * over bits so as to prevent surprises.
> - */
> -static void clamp_last_byte(uint8_t *bp, unsigned int nbits)
> -{
> -	unsigned int remainder = nbits % 8;
> -
> -	if (remainder)
> -		bp[nbits/8] &= (1U << remainder) - 1;
> -}
> -
>  int __bitmap_empty(const unsigned long *bitmap, unsigned int bits)
>  {
>  	int k, lim = bits/BITS_PER_LONG;
> @@ -338,7 +323,6 @@ static void bitmap_long_to_byte(uint8_t
>  			nbits -= 8;
>  		}
>  	}
> -	clamp_last_byte(bp, nbits);
>  }
>  
>  static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
> @@ -359,12 +343,11 @@ static void bitmap_byte_to_long(unsigned
>  
>  #elif defined(__LITTLE_ENDIAN)
>  
> -static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
> -				unsigned int nbits)
> -{
> -	memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
> -	clamp_last_byte(bp, nbits);
> -}
> +#define LITTLE_ENDIAN 1 /* For IS_ENABLED(). */
> +
> +/* Unused function, but a declaration is needed. */
> +void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
> +			 unsigned int nbits);
>  
>  static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
>  				unsigned int nbits)
> @@ -384,22 +367,46 @@ int bitmap_to_xenctl_bitmap(struct xenct
>      uint8_t zero = 0;
>      int err = 0;
>      unsigned int xen_bytes = DIV_ROUND_UP(nbits, BITS_PER_BYTE);
> -    uint8_t *bytemap = xmalloc_array(uint8_t, xen_bytes);
> +    const uint8_t *bytemap;
> +    uint8_t last, *buf = NULL;
>  
> -    if ( !bytemap )
> -        return -ENOMEM;
> +    if ( !IS_ENABLED(LITTLE_ENDIAN) )
> +    {
> +        buf = xmalloc_array(uint8_t, xen_bytes);
> +        if ( !buf )
> +            return -ENOMEM;
> +
> +        bitmap_long_to_byte(buf, bitmap, nbits);
> +
> +        bytemap = buf;
> +    }
> +    else
> +        bytemap = (const uint8_t *)bitmap;
>  
>      guest_bytes = DIV_ROUND_UP(xenctl_bitmap->nr_bits, BITS_PER_BYTE);
>      copy_bytes  = min(guest_bytes, xen_bytes);
>  
> -    bitmap_long_to_byte(bytemap, bitmap, nbits);
> +    if ( copy_bytes > 1 &&
> +         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes - 1) )
> +        err = -EFAULT;
> +
> +    /*
> +     * If a bitmap has a number of bits which is not a multiple of 8 then the
> +     * last few bits of the last byte of the bitmap can be unexpectedly set,
> +     * which can confuse consumers (e.g. in the tools), who also may round up
> +     * their loops to 8 bits. Ensure we clear those left over bits so as to
> +     * prevent surprises.
> +     */
> +    last = bytemap[nbits / 8];
> +    if ( nbits % 8 )
> +        last &= (1U << (nbits % 8)) - 1;
> +
> +    xfree(buf);
>  
>      if ( copy_bytes &&
> -         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
> +         copy_to_guest_offset(xenctl_bitmap->bitmap, copy_bytes - 1, &last, 1) )
>          err = -EFAULT;
>  
> -    xfree(bytemap);
> -
>      for ( i = copy_bytes; !err && i < guest_bytes; i++ )
>          if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )
>              err = -EFAULT;
> 

Re: [PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Jan Beulich]

On 02.04.2025 02:14, Stefano Stabellini wrote:
> On Tue, 1 Apr 2025, Jan Beulich wrote:
>> From: Stefano Stabellini <stefano.stabellini@amd.com>
>>
>> The little endian implementation of bitmap_to_xenctl_bitmap leads to
>> unnecessary xmallocs and xfrees. Given that Xen only supports little
>> endian architectures, it is worth optimizing.
>>
>> This patch removes the need for the xmalloc on little endian
>> architectures.
>>
>> Remove clamp_last_byte as it is only called once and only needs to
>> modify one byte. Inline it.
>>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v5: Fix IS_ENABLED() use. Keep more code common. Move comment.
>>     Convert LE bitmap_long_to_byte() to just a declaration.
> 
> Thanks Jan, I looked at your version carefully and it looks correct to
> me. I could give my reviewed-by but it looks weird given that I am also
> the first author.

./MAINTAINERS has a section specifically on this situation, starting with
"In the case where two people collaborate on a patch, ..."

> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks.

Jan

Re: [PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Andrew Cooper]

On 01/04/2025 11:09 am, Jan Beulich wrote:
> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -359,12 +343,11 @@ static void bitmap_byte_to_long(unsigned
>  
>  #elif defined(__LITTLE_ENDIAN)
>  
> -static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
> -				unsigned int nbits)
> -{
> -	memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
> -	clamp_last_byte(bp, nbits);
> -}
> +#define LITTLE_ENDIAN 1 /* For IS_ENABLED(). */

I guess I can fix this in my bswap series by giving __LITTLE_ENDIAN the
value of 1 when it is defined ?

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This is very very long overdue.

~Andrew

Re: [PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Jan Beulich]

On 02.04.2025 11:21, Andrew Cooper wrote:
> On 01/04/2025 11:09 am, Jan Beulich wrote:
>> --- a/xen/common/bitmap.c
>> +++ b/xen/common/bitmap.c
>> @@ -359,12 +343,11 @@ static void bitmap_byte_to_long(unsigned
>>  
>>  #elif defined(__LITTLE_ENDIAN)
>>  
>> -static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
>> -				unsigned int nbits)
>> -{
>> -	memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
>> -	clamp_last_byte(bp, nbits);
>> -}
>> +#define LITTLE_ENDIAN 1 /* For IS_ENABLED(). */
> 
> I guess I can fix this in my bswap series by giving __LITTLE_ENDIAN the
> value of 1 when it is defined ?

Hmm, that's an option, yes. Yet then I didn't pay close enough attention
to that part of the respective patch in that series, as I was blindly
assuming it to instead be

#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
# define __LITTLE_ENDIAN __ORDER_LITTLE_ENDIAN__
#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
# define __BIG_ENDIAN __ORDER_BIG_ENDIAN__
#endif

to keep the values of the symbols unaltered.

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan

Re: [PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Andrew Cooper]

On 02/04/2025 10:30 am, Jan Beulich wrote:
> On 02.04.2025 11:21, Andrew Cooper wrote:
>> On 01/04/2025 11:09 am, Jan Beulich wrote:
>>> --- a/xen/common/bitmap.c
>>> +++ b/xen/common/bitmap.c
>>> @@ -359,12 +343,11 @@ static void bitmap_byte_to_long(unsigned
>>>  
>>>  #elif defined(__LITTLE_ENDIAN)
>>>  
>>> -static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
>>> -				unsigned int nbits)
>>> -{
>>> -	memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
>>> -	clamp_last_byte(bp, nbits);
>>> -}
>>> +#define LITTLE_ENDIAN 1 /* For IS_ENABLED(). */
>> I guess I can fix this in my bswap series by giving __LITTLE_ENDIAN the
>> value of 1 when it is defined ?
> Hmm, that's an option, yes. Yet then I didn't pay close enough attention
> to that part of the respective patch in that series, as I was blindly
> assuming it to instead be
>
> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> # define __LITTLE_ENDIAN __ORDER_LITTLE_ENDIAN__
> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> # define __BIG_ENDIAN __ORDER_BIG_ENDIAN__
> #endif
>
> to keep the values of the symbols unaltered.

I saw no value doing that, after auditing that it was only ever used in
an #ifdef kind of way.  IS_ENABLED() is an extension of the same
concept, but requiring a 1 to work.

~Andrew

Re: [PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Jan Beulich]

On 02.04.2025 11:36, Andrew Cooper wrote:
> On 02/04/2025 10:30 am, Jan Beulich wrote:
>> On 02.04.2025 11:21, Andrew Cooper wrote:
>>> On 01/04/2025 11:09 am, Jan Beulich wrote:
>>>> --- a/xen/common/bitmap.c
>>>> +++ b/xen/common/bitmap.c
>>>> @@ -359,12 +343,11 @@ static void bitmap_byte_to_long(unsigned
>>>>  
>>>>  #elif defined(__LITTLE_ENDIAN)
>>>>  
>>>> -static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
>>>> -				unsigned int nbits)
>>>> -{
>>>> -	memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
>>>> -	clamp_last_byte(bp, nbits);
>>>> -}
>>>> +#define LITTLE_ENDIAN 1 /* For IS_ENABLED(). */
>>> I guess I can fix this in my bswap series by giving __LITTLE_ENDIAN the
>>> value of 1 when it is defined ?
>> Hmm, that's an option, yes. Yet then I didn't pay close enough attention
>> to that part of the respective patch in that series, as I was blindly
>> assuming it to instead be
>>
>> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> # define __LITTLE_ENDIAN __ORDER_LITTLE_ENDIAN__
>> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> # define __BIG_ENDIAN __ORDER_BIG_ENDIAN__
>> #endif
>>
>> to keep the values of the symbols unaltered.
> 
> I saw no value doing that, after auditing that it was only ever used in
> an #ifdef kind of way.  IS_ENABLED() is an extension of the same
> concept, but requiring a 1 to work.

Fair enough then, and a (tiny) simplification here. I'll see about getting the
change here in, so you can re-base over it.

Jan