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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <JBeulich@xxxxxxxx>
> Sent: 03 July 2019 10:39
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH 1/3] xmalloc: stop using a magic '1' in alignment padding
> 
> 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).
> 

Yes, the code is pretty ancient. There's a whole bunch of cleanup/style 
adjustments (e.g. u32 -> uint32_t) too. I left this one for consistency.

> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> 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.

Oh yes, sorry I missed that.

> 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.
> 

Yes, please go ahead and fix on commit.

  Paul

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