| Message ID | 20190702163840.2107-2-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xmalloc patches | expand |
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@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> 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
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 03 July 2019 10:39 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; > Wei Liu <wl@xen.org> > 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@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> 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
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index 2076953ac4..6d889b7bdc 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -595,7 +595,7 @@ void *_xmalloc(unsigned long size, unsigned long align) char *q = (char *)p + pad; struct bhdr *b = (struct bhdr *)(q - BHDR_OVERHEAD); ASSERT(q > (char *)p); - b->size = pad | 1; + b->size = pad | FREE_BLOCK; p = q; } @@ -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); b = (struct bhdr *)((char *)p - BHDR_OVERHEAD); - ASSERT(!(b->size & 1)); + ASSERT(!(b->size & FREE_BLOCK)); } xmem_pool_free(p, xenpool);
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. Also, whilst in the neighbourhood, it removes a stray space after a cast. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wl@xen.org> --- xen/common/xmalloc_tlsf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)