Re: [Xen-devel] [PATCH V4 4/8] xen/common: Introduce _xrealloc function

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2019 17:35, Oleksandr Tyshchenko wrote:
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -598,6 +598,58 @@ void *_xzalloc(unsigned long size, unsigned long align)
>      return p ? memset(p, 0, size) : p;
>  }
>  
> +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;

While the present MAX_ORDER setting will prevent allocations of
4GiB or above from succeeding, may I ask that you don't introduce
latent issues in case MAX_ORDER would ever need bumping?

> +    else
> +    {
> +        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));
> +        }

This matches the respective xfree() code fragment, and needs to
remain in sync. Which suggests introducing a helper function
instead of duplicating the code. And please omit the unnecessary
casts to char *.

> +        curr_size = b->size & BLOCK_SIZE_MASK;

_xmalloc() has

        b->size = pad | FREE_BLOCK;

i.e. aiui what you calculate above is the padding size, not the
overall block size.

> +    }
> +
> +    ASSERT((align & (align - 1)) == 0);
> +    if ( align < MEM_ALIGN )
> +        align = MEM_ALIGN;
> +    tmp_size = size + align - MEM_ALIGN;
> +
> +    if ( tmp_size < PAGE_SIZE )
> +        tmp_size = ( tmp_size < MIN_BLOCK_SIZE ) ? MIN_BLOCK_SIZE :

Stray blanks inside parentheses.

> +            ROUNDUP_SIZE(tmp_size);
> +
> +    if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 )
> +        return ptr; /* the size and alignment fit in already allocated space 
> */

You also don't seem to ever update ptr in case you want to use the
(head) padding, i.e. you'd hand back a pointer to a block which the
caller would assume extends past its actual end. I think you want
to calculate the new tentative pointer (taking the requested
alignment into account), and only from that calculate curr_size
(which perhaps would better be named "usable" or "space" or some
such). Obviously the (head) padding block may need updating, too.

> +    p = _xmalloc(size, align);
> +    if ( p )
> +    {
> +        memcpy(p, ptr, min(curr_size, size));
> +        xfree(ptr);
> +    }
> +
> +    return p;
> +}

As a final remark - did you consider zero(?)-filling the tail
portion? While C's realloc() isn't specified to do so, since there's
no (not going to be a) zeroing counterpart, doing so may be safer
overall.

Jan

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