| Message ID | 694eac75-e872-4ba0-80ed-95c14cd11f5e@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] xen: simplify bitmap_to_xenctl_bitmap for little endian | expand |
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; >
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
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
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
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
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
--- 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;