[CRIU] Re: [PATCH cr 5/6] pstree: allocate rst parts from a shared region

[Pavel Emelyanov]

On 10/02/2012 09:17 PM, Andrey Vagin wrote:
> On Tue, Oct 02, 2012 at 07:58:02PM +0400, Pavel Emelyanov wrote:
>> On 10/02/2012 06:07 PM, Andrey Vagin wrote:
>>> The next patch will add a futex, which will be used for restoring
>>> shared fd tables.
>>>
>>> Signed-off-by: Andrey Vagin <avagin at openvz.org>
>>> ---
>>>  include/pstree.h |    2 +-
>>>  pstree.c         |    8 +++++++-
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/pstree.h b/include/pstree.h
>>> index 0790b49..2da7176 100644
>>> --- a/include/pstree.h
>>> +++ b/include/pstree.h
>>> @@ -20,7 +20,7 @@ struct pstree_item {
>>>  	int			state;		/* TASK_XXX constants */
>>>  	int			nr_threads;	/* number of threads */
>>>  	struct pid		*threads;	/* array of threads */
>>> -	struct rst_info		rst[0];
>>> +	struct rst_info		*rst;
>>>  };
>>>  
>>>  extern void free_pstree(struct pstree_item *root_item);
>>> diff --git a/pstree.c b/pstree.c
>>> index 8df5a63..1672b26 100644
>>> --- a/pstree.c
>>> +++ b/pstree.c
>>> @@ -33,10 +33,16 @@ struct pstree_item *__alloc_pstree_item(bool rst)
>>>  {
>>>  	struct pstree_item *item;
>>>  
>>> -	item = xzalloc(sizeof(*item) + (rst ? sizeof(item->rst[0]) : 0));
>>> +	item = xzalloc(sizeof(*item));
>>>  	if (!item)
>>>  		return NULL;
>>>  
>>> +	if (rst) {
>>> +		item->rst = shmalloc(sizeof(item->rst[0]));
>>
>> The smalloc has caveat -- it's hared with children _only_. Does that suit us?
> Yes, because all pstree items are allocated in crtools before retoring
> a root task.

Good. Please, put this as a comment to this shmalloc.

>> Thing #2. The rst_info is generic rst_info, what if we add there something which
>> is not fdtable related? Things will get broken.
> I don't understand this point. Here are a few list heads already. Now
> rst_info is used only by owner and only fdt_lock can be used by another
> task, so we don't have problem.

Then rename it to reflect the fact, that it's fdtable-specific since now on.

>>
>>> +		if (!item->rst)
>>> +			return NULL;
>>> +	}
>>> +
>>>  	INIT_LIST_HEAD(&item->children);
>>>  	item->threads = NULL;
>>>  	item->nr_threads = 0;
>>>
>>
>>
> .
>