Re: [Xen-devel] [PATCH V3 3/8] xen/common: Introduce _xrealloc function

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Oleksandr <olekstysh@xxxxxxxxx>
> Sent: 21 August 2019 15:36
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: sstabellini@xxxxxxxxxx; Wei Liu <wl@xxxxxxx>; Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Oleksandr Tyshchenko
> <oleksandr_tyshchenko@xxxxxxxx>; julien.grall@xxxxxxx; Jan Beulich 
> <jbeulich@xxxxxxxx>;
> Volodymyr_Babchuk@xxxxxxxx
> Subject: Re: [Xen-devel] [PATCH V3 3/8] xen/common: Introduce _xrealloc 
> function
> 
> 
> On 21.08.19 11:09, Paul Durrant wrote:
> 
> Hi, Paul
> 
> >>   }
> >>
> >> +void *_xrealloc(void *ptr, unsigned long size, unsigned long align)
> >> +{
> >> +    unsigned long curr_size, tmp_size;
> >> +    void *p;
> >> +
> >> +    if ( !size )
> >> +    {
> >> +        xfree(ptr);
> >> +        return ZERO_BLOCK_PTR;
> >> +    }
> >> +
> >> +    if ( ptr == NULL || ptr == ZERO_BLOCK_PTR )
> >> +        return _xmalloc(size, align);
> >> +
> >> +    if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) )
> >> +        curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT;
> >> +    else
> >> +    {
> >> +        struct bhdr *b;
> >> +        b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD);
> >> +        curr_size = b->size & BLOCK_SIZE_MASK;
> >> +    }
> > That seconds clause is not going to give you the block size if the previous 
> > allocation had alignment
> padding. You'll need to check the FREE_BLOCK bit to tell whether it's a real 
> block header or the
> 'fake' alignment header and then maybe walk backwards onto the real header. 
> See the code in xfree().
> 
> Indeed. Thank you for the pointer.
> 
> 
> > You should also check whether the new requested alignment is compatible 
> > with the existing block
> alignment
> 
> 
> I am wondering:
> 
> In case when we use only type-safe construction for the basic allocation
> (xmalloc) and type-safe construction for the re-allocation
> (xrealloc_flex_struct) over the same pointer of the correct type, will
> we get a possible alignment incompatibility?

You shouldn't but you're trusting that no-one will want to call the function 
directly with a higher alignment.
 
> 
> 
> This is how I see it:
> 
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index eecae2e..f90f453 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -616,8 +616,15 @@ void *_xrealloc(void *ptr, unsigned long size,
> unsigned long align)
>           curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT;
>       else
>       {
> -        struct bhdr *b;
> -        b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD);
> +        struct bhdr *b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD);
> +
> +        if ( b->size & FREE_BLOCK )
> +        {
> +            p = (char *)ptr - (b->size & ~FREE_BLOCK);
> +            b = (struct bhdr *)((char *)p - BHDR_OVERHEAD);
> +            ASSERT(!(b->size & FREE_BLOCK));
> +        }
> +
>           curr_size = b->size & BLOCK_SIZE_MASK;

Yes, that looks ok for getting the size right...

>       }
> 
> @@ -630,8 +637,8 @@ void *_xrealloc(void *ptr, unsigned long size,
> unsigned long align)
>           tmp_size = ( tmp_size < MIN_BLOCK_SIZE ) ? MIN_BLOCK_SIZE :
>               ROUNDUP_SIZE(tmp_size);
> 
> -    if ( tmp_size <= curr_size ) /* fits in current block */
> -        return ptr;
> +    if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 )
> +        return ptr; /* fits in current block */

...and that should deal with the alignment too (although you may want to 
mention the alignment part in the comment too)

> 
>       p = _xmalloc(size, align);
>       if ( p )
> 
> (END)
> 
> 
> Did I get your point correct?
> 

Yes, with those changes I think it will look ok.

  Paul

> 
> --
> Regards,
> 
> Oleksandr Tyshchenko

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