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

[Jan Beulich <jbeulich@suse.com>]

On 24.09.2019 17:30, Oleksandr Tyshchenko wrote:
> Changes V4 -> V5:
>     - avoid possible truncation with allocations of 4GiB or above
>     - introduce helper functions add(strip)_padding to avoid
>       duplicating the code
>     - omit the unnecessary casts, change u32 to uint32_t
>       when moving the code
>     - use _xzalloc instead of _xmalloc to get the tail
>       portion zeroed

I'm sorry, but no, this is wasteful: You write the initialized
portion of the block twice this way, when once is fully
sufficient (but see below).

> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -554,10 +554,40 @@ static void tlsf_init(void)
>  #define ZERO_BLOCK_PTR ((void *)-1L)
>  #endif
>  
> +static void *strip_padding(void *p)
> +{
> +    struct bhdr *b = p - BHDR_OVERHEAD;
> +
> +    if ( b->size & FREE_BLOCK )
> +    {
> +        p -= b->size & ~FREE_BLOCK;
> +        b = p - BHDR_OVERHEAD;
> +        ASSERT(!(b->size & FREE_BLOCK));
> +    }
> +
> +    return p;
> +}
> +
> +static void *add_padding(void *p, unsigned long align)
> +{
> +    uint32_t pad;

A fixed width type is inappropriate here anyway - unsigned int would
suffice. Judging from the incoming parameters, unsigned long would
be more future proof; alternatively the "align" parameter could be
"unsigned int", since we don't allow such huge allocations anyway
(but I agree that adjusting this doesn't really belong in the patch
here).

> @@ -598,10 +621,70 @@ void *_xzalloc(unsigned long size, unsigned long align)
>      return p ? memset(p, 0, size) : p;
>  }
>  
> -void xfree(void *p)
> +void *_xrealloc(void *ptr, unsigned long size, unsigned long align)
>  {
> -    struct bhdr *b;
> +    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);

This is inconsistent - you use _xzalloc() further down (and too
aggressively imo, as said). Here use of that function would then
be indicated. However, ...

> +    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 :
> +            ROUNDUP_SIZE(tmp_size);
> +
> +    if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) )
> +    {
> +        curr_size = (unsigned long)PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT;
> +
> +        if ( size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 )
> +            return ptr;

... I only now realize that in a case like this one you can't really
zero-fill the tail portion that's extending beyond the original
request. Hence I think the just-in-case zero filling would better be
dropped again (and the _xmalloc() use above is fine).

Note that with the fix done here you don't need tmp_size anymore
outside of ...

> +    }
> +    else
> +    {

... this block. Please move its calculation (and declaration) here.

> +        struct bhdr *b;
> +
> +        /* Strip alignment padding. */
> +        p = strip_padding(ptr);
> +
> +        b = p - BHDR_OVERHEAD;
> +        curr_size = b->size & BLOCK_SIZE_MASK;
> +
> +        if ( tmp_size <= curr_size )
> +        {
> +            /* Add alignment padding. */
> +            p = add_padding(p, align);
> +
> +            ASSERT(((unsigned long)p & (align - 1)) == 0);

Since another rev is going to be needed anyway - here and above
please prefer ! over == 0.

Jan

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