diff mbox series

xen: simplify bitmap_to_xenctl_bitmap for little endian

Message ID alpine.DEB.2.22.394.2503182002160.2000798@ubuntu-linux-20-04-desktop (mailing list archive)
State New
Headers show
Series xen: simplify bitmap_to_xenctl_bitmap for little endian | expand

Commit Message

Stefano Stabellini March 19, 2025, 3:03 a.m. UTC 
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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>

Comments

Jan Beulich March 19, 2025, 9:54 a.m. UTC | #1 
On 19.03.2025 04:03, Stefano Stabellini wrote:
> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -384,21 +384,26 @@ 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 *bytemap = (uint8_t *)bitmap;

Not only Misra dislikes casting away of const-ness.

>      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) )
> +    {
> +        bytemap = xmalloc_array(uint8_t, xen_bytes);
> +        if ( !bytemap )
> +            return -ENOMEM;
> +
> +        bitmap_long_to_byte(bytemap, bitmap, nbits);
> +    }

Where did the clamp_last_byte() go in the little-endian case?

>      if ( copy_bytes &&
>           copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
>          err = -EFAULT;
>  
> -    xfree(bytemap);
> +    if ( IS_ENABLED(__BIG_ENDIAN) )
> +        xfree(bytemap);
>  
>      for ( i = copy_bytes; !err && i < guest_bytes; i++ )
>          if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )

What about xenctl_bitmap_to_bitmap()?

Jan
Stefano Stabellini March 20, 2025, 12:57 a.m. UTC | #2 
On Wed, 19 Mar 2025, Jan Beulich wrote:
> What about xenctl_bitmap_to_bitmap()?
 
Let me see first if I managed to handle bitmap_to_xenctl_bitmap well.

---

[PATCH v2] xen: simplify bitmap_to_xenctl_bitmap for little endian

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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
Changes in v2:
- don't remove const
- handle clamp_last_byte for little endian

diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index 3da63a32a6..e9876ee5a6 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -384,21 +384,33 @@ 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;
+    bool alloc = (bitmap[nbits/8] & ((1U << (nbits % 8)) - 1)) ||
+                 IS_ENABLED(__BIG_ENDIAN);
 
     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 ( alloc )
+    {
+        uint8_t *bytemap = xmalloc_array(uint8_t, xen_bytes);
+        if ( !bytemap )
+            return -ENOMEM;
 
-    if ( copy_bytes &&
-         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
-        err = -EFAULT;
+        bitmap_long_to_byte(bytemap, bitmap, nbits);
 
-    xfree(bytemap);
+        if ( copy_bytes &&
+             copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
+            err = -EFAULT;
+
+        xfree(bytemap);
+    }
+    else
+    {
+        const uint8_t *bytemap = (const uint8_t *)bitmap;
+        if ( copy_bytes &&
+             copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
+            err = -EFAULT;
+    }
 
     for ( i = copy_bytes; !err && i < guest_bytes; i++ )
         if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )
Jan Beulich March 20, 2025, 7:26 a.m. UTC | #3 
On 20.03.2025 01:57, Stefano Stabellini wrote:
> On Wed, 19 Mar 2025, Jan Beulich wrote:
>> What about xenctl_bitmap_to_bitmap()?
>  
> Let me see first if I managed to handle bitmap_to_xenctl_bitmap well.

Well, the code looks correct to me, but the description now has gone
stale. I also wonder whether with that extra restriction the optimization
is then actually worth it. Just one further nit:

> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -384,21 +384,33 @@ 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;
> +    bool alloc = (bitmap[nbits/8] & ((1U << (nbits % 8)) - 1)) ||

Blanks missing around / here.

Jan
diff mbox series

Patch

diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index 3da63a32a6..38d77fc876 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -384,21 +384,26 @@  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 *bytemap = (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 ( IS_ENABLED(__BIG_ENDIAN) )
+    {
+        bytemap = xmalloc_array(uint8_t, xen_bytes);
+        if ( !bytemap )
+            return -ENOMEM;
+
+        bitmap_long_to_byte(bytemap, bitmap, nbits);
+    }
 
     if ( copy_bytes &&
          copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
         err = -EFAULT;
 
-    xfree(bytemap);
+    if ( IS_ENABLED(__BIG_ENDIAN) )
+        xfree(bytemap);
 
     for ( i = copy_bytes; !err && i < guest_bytes; i++ )
         if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )