Re: [Xen-devel] [PATCH 1/3] xmalloc: stop using a magic '1' in alignment padding

[Jan Beulich <JBeulich@xxxxxxxx>]

On 02.07.2019 18:38, Paul Durrant wrote:
> Alignment padding inserts a pseudo block header in front of the allocation,
> sets its size field to the pad size and then ORs in 1, which is equivalent
> to marking it as a free block, so that xfree() can distinguish it from a
> real block header.
> 
> This patch simply replaces the magic '1' with the defined 'FREE_BLOCK' to
> make it more obvious what's going on.

Hmm, that's still an abuse of some sort, I think. FREE_BLOCK
(together with USED_BLOCK, PREV_FREE, and PREV_USED) serve
block splitting and re-combination, which isn't strictly the
case here. But yes, I guess (ab)using the manifest constants is
still better than (ab)using the literal numbers.

> Also, whilst in the neighbourhood, it removes a stray space after a cast.

An option would have been to drop the cast altogether. The code
here appears to assume that void pointer arithmetic is not
allowed (as is indeed the case in plain C).

> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one further adjustment:

> @@ -638,12 +638,12 @@ void xfree(void *p)
>       }
>   
>       /* Strip alignment padding. */
> -    b = (struct bhdr *)((char *) p - BHDR_OVERHEAD);
> -    if ( b->size & 1 )
> +    b = (struct bhdr *)((char *)p - BHDR_OVERHEAD);
> +    if ( b->size & FREE_BLOCK )
>       {
>           p = (char *)p - (b->size & ~1u);

This ~1u also wants to become ~FREE_BLOCK then. I guess the
change is easy enough to make while committing; I don't
expect the loss of the u suffix to actually cause any
problems. In fact its presence was not a problem only
because ->size can't get very large and is of u32 type.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel