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

From: Jan Beulich <jbeulich@suse.com>
To: Oleksandr <olekstysh@gmail.com>
Cc: sstabellini@kernel.org, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	julien.grall@arm.com, Paul Durrant <paul.durrant@citrix.com>,
	xen-devel@lists.xenproject.org, Volodymyr_Babchuk@epam.com
Subject: Re: [Xen-devel] [PATCH V4 4/8] xen/common: Introduce _xrealloc function
Date: Mon, 16 Sep 2019 17:24:51 +0200	[thread overview]
Message-ID: <93811967-b49d-7a86-6d19-647cd0e8d1dd@suse.com> (raw)
In-Reply-To: <0021c5ab-457e-7cbf-a5c7-7d8676503116@gmail.com>

On 16.09.2019 17:03, Oleksandr wrote:
> On 16.09.19 13:13, Jan Beulich wrote:
>> 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;

Yes.

>>> +            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?

The very padding talked about earlier. I did add "(head)" to clarify
it's that specific case - after all tail padding is far more common.

Jan

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