[CRIU] [PATCH 1/2] rst-malloc: return aligned pointers to sizeof(void *) (v3)

[Pavel Emelyanov]

On 01/27/2016 08:29 PM, Andrew Vagin wrote:
> On Wed, Jan 27, 2016 at 04:29:43PM +0300, Pavel Emelyanov wrote:
>> On 01/26/2016 10:58 PM, Andrey Vagin wrote:
>>> From: Andrew Vagin <avagin at virtuozzo.com>
>>>
>>> Stas found that if we don't align a pointer,
>>> futex and atomic operations can fail.
>>>
>>> v2: don't hard-code the size of void *
>>> v3: add a function to allocate memory without gaps with
>>>     a privious slice. It's used to allocate arrays.
>>>
>>> Cc: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
>>> Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
>>> ---
>>>  include/rst-malloc.h |  1 +
>>>  rst-malloc.c         | 23 ++++++++++++++++++++++-
>>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/rst-malloc.h b/include/rst-malloc.h
>>> index 8c48f27..f205622 100644
>>> --- a/include/rst-malloc.h
>>> +++ b/include/rst-malloc.h
>>> @@ -58,6 +58,7 @@ extern void *rst_mem_remap_ptr(unsigned long pos, int type);
>>>   * last object can be freed (pop-ed from buffer).
>>>   */
>>>  extern void *rst_mem_alloc(unsigned long size, int type);
>>> +extern void *rst_mem_alloc_cont(unsigned long size, int type);
>>>  extern void rst_mem_free_last(int type);
>>>  /*
>>>   * Routines to remap SHREMAP and PRIVATE into restorer address space
>>> diff --git a/rst-malloc.c b/rst-malloc.c
>>> index 14e0b41..7685430 100644
>>> --- a/rst-malloc.c
>>> +++ b/rst-malloc.c
>>> @@ -128,6 +128,9 @@ unsigned long rst_mem_cpos(int type)
>>>  {
>>>  	struct rst_mem_type_s *t = &rst_mems[type];
>>>  	BUG_ON(!t->remapable || !t->enabled);
>>> +
>>> +	t->free_mem = (void *) round_up((unsigned long)t->free_mem, sizeof(void *));
>>
>> Should this hunk be here? Why?
> 
> Yes, it should. We want to aligne a start address of arrays. It's about
> a case when rst_mem_alloc_cont is used.

It's wrong place for such a thing. The rst_mem_cpos() should be "pure function",
i.e. do not change anything inside.

>>
>>> +
>>>  	return t->free_mem - t->buf;
>>>  }
>>>  
>>> @@ -138,7 +141,7 @@ void *rst_mem_remap_ptr(unsigned long pos, int type)
>>>  	return t->buf + pos;
>>>  }
>>>  
>>> -void *rst_mem_alloc(unsigned long size, int type)
>>> +static void *__rst_mem_alloc(unsigned long size, int type)
>>>  {
>>>  	struct rst_mem_type_s *t = &rst_mems[type];
>>>  	void *ret;
>>> @@ -158,6 +161,24 @@ void *rst_mem_alloc(unsigned long size, int type)
>>>  	return ret;
>>>  }
>>>  
>>> +void *rst_mem_alloc(unsigned long size, int type)
>>> +{
>>> +	struct rst_mem_type_s *t = &rst_mems[type];
>>> +
>>> +	t->free_mem = (void *) round_up((unsigned long)t->free_mem, sizeof(void *));
>>
>> Any guarantee you don't overflow the free_mem beyond the allocated memory buffer?
> 
> __rst_mem_alloc will check and call grow() if it's required.

Hm... Then the respective BUG_ON() should be here.

>>
>>> +
>>> +	return __rst_mem_alloc(size, type);
>>> +}
>>> +
>>> +/* Allocate memory without gaps with a previous slice */
>>> +void *rst_mem_alloc_cont(unsigned long size, int type)
>>
>> I'd better make a call titles rst_mem_alloc_aligned().
> 
> rst_mem_alloc_NOT_align
> 
> I don't want to rename rst_mem_alloc, because ussualy this sort of
> functions (e.g. malloc) returns alligned addresses.
> 
> I was thinking about this, but rst_mem_alloc_cont looks better for met.
> 
> Maybe we need to add rst_mem_realloc() instead of rst_mem_alloc_cont.

Or rst_mem_grow(). OK, let's do that.

>>
>>> +{
>>> +	struct rst_mem_type_s *t = &rst_mems[type];
>>> +	BUG_ON(!t->remapable);
>>> +
>>> +	return __rst_mem_alloc(size, type);
>>> +}
>>> +
>>>  void rst_mem_free_last(int type)
>>>  {
>>>  	struct rst_mem_type_s *t = &rst_mems[type];
>>>
>>
> .
>