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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 16.09.19 13:13, Jan Beulich wrote:

Hi, Jan

> 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?
Sure (I assume, you are talking about possible truncation):

if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) )
    curr_size = (unsigned long)PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT;


> > +    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 *.
Sounds reasonable, will do.


> > +        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.
I have to admit that I am not familiar with allocator internals enough, but

I meant to calculate overall block size (the alignment padding is 
stripped if present)...

> > +    }
> > +
> > +    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.
ok


> > +            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.
I am afraid I don't completely understand your point here. And sorry for 
the maybe naive question, but what is the "(head) padding" here?

> > +    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.
Probably, worth doing. Will zero it.


--
Regards,

Oleksandr Tyshchenko


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