[CRIU] [PATCH 3/5] pstree: prepare to store all pid-s in rb-tree

Pavel Emelyanov xemul at virtuozzo.com
Wed Mar 9 23:33:50 PST 2016


On 03/09/2016 09:43 PM, Andrew Vagin wrote:
> On Wed, Mar 09, 2016 at 02:02:39PM +0300, Pavel Emelyanov wrote:
>> On 02/19/2016 09:13 PM, Andrey Vagin wrote:
>>> From: Andrew Vagin <avagin at virtuozzo.com>
>>>
>>> It will allow us to find a free pid and will speed up
>>> searching an item by pid.
>>>
>>> The current algorithm to searching a free pid may return
>>> a value bigger that kernel.max_pid.
>>>
>>> Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
>>> ---
>>>  criu/files-reg.c      | 17 +++++++--------
>>>  criu/include/image.h  |  2 ++
>>>  criu/include/pid.h    |  3 +++
>>>  criu/include/pstree.h |  2 ++
>>>  criu/pstree.c         | 58 +++++++++++++++++++++++++++++++++++++++++++++++----
>>>  5 files changed, 69 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/criu/files-reg.c b/criu/files-reg.c
>>> index 96921b8..e47cb76 100644
>>> --- a/criu/files-reg.c
>>> +++ b/criu/files-reg.c
>>> @@ -357,18 +357,17 @@ static int open_remap_dead_process(struct reg_file_info *rfi,
>>>  {
>>>  	struct pstree_item *helper;
>>>  
>>> -	for_each_pstree_item(helper) {
>>> -		/* don't need to add multiple tasks */
>>> -		if (helper->pid.virt == rfe->remap_id) {
>>> -			pr_info("Skipping helper for restoring /proc/%d; pid exists\n", rfe->remap_id);
>>> -			return 0;
>>> -		}
>>> -	}
>>> -
>>> -	helper = alloc_pstree_helper();
>>> +	helper = insert_item(rfe->remap_id);
>>>  	if (!helper)
>>>  		return -1;
>>>  
>>> +	if (helper->pid.state != TASK_UNDEF) {
>>> +		pr_info("Skipping helper for restoring /proc/%d; pid exists\n", rfe->remap_id);
>>> +		return 0;
>>> +	}
>>> +
>>> +	init_pstree_helper(helper);
>>> +
>>>  	helper->sid = root_item->sid;
>>>  	helper->pgid = root_item->pgid;
>>>  	helper->pid.virt = rfe->remap_id;
>>> diff --git a/criu/include/image.h b/criu/include/image.h
>>> index 305febf..f141915 100644
>>> --- a/criu/include/image.h
>>> +++ b/criu/include/image.h
>>> @@ -112,10 +112,12 @@
>>>  
>>>  #define TASK_COMM_LEN 16
>>>  
>>> +#define TASK_UNDEF		0x0
>>>  #define TASK_ALIVE		0x1
>>>  #define TASK_DEAD		0x2
>>>  #define TASK_STOPPED		0x3
>>>  #define TASK_HELPER		0x4
>>> +#define TASK_THREAD		0x5
>>>  
>>>  #define CR_PARENT_LINK "parent"
>>>  
>>> diff --git a/criu/include/pid.h b/criu/include/pid.h
>>> index 2a709a2..533be88 100644
>>> --- a/criu/include/pid.h
>>> +++ b/criu/include/pid.h
>>> @@ -2,6 +2,7 @@
>>>  #define __CR_PID_H__
>>>  
>>>  #include "stdbool.h"
>>> +#include "rbtree.h"
>>>  
>>>  struct pid {
>>>  	/*
>>> @@ -19,6 +20,8 @@ struct pid {
>>>  	pid_t virt;
>>>  
>>>  	int state;	/* TASK_XXX constants */
>>> +
>>> +	struct rb_node node;
>>>  };
>>>  
>>>  /*
>>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
>>> index fcc1158..7bef810 100644
>>> --- a/criu/include/pstree.h
>>> +++ b/criu/include/pstree.h
>>> @@ -68,6 +68,8 @@ extern struct pstree_item *__alloc_pstree_item(bool rst);
>>>  extern struct pstree_item *alloc_pstree_helper(void);
>>>  extern void init_pstree_helper(struct pstree_item *ret);
>>>  
>>> +extern struct pstree_item *insert_item(pid_t pid);
>>> +
>>>  extern struct pstree_item *root_item;
>>>  extern struct pstree_item *pstree_item_next(struct pstree_item *item);
>>>  #define for_each_pstree_item(pi) \
>>> diff --git a/criu/pstree.c b/criu/pstree.c
>>> index 2688547..1be8c48 100644
>>> --- a/criu/pstree.c
>>> +++ b/criu/pstree.c
>>> @@ -17,6 +17,7 @@
>>>  #include "images/pstree.pb-c.h"
>>>  
>>>  struct pstree_item *root_item;
>>> +struct rb_root root_rb;
>>
>> Bad name for global variable.
> 
> It should be static
> 
>>
>>>  
>>>  #define CLONE_ALLNS     (CLONE_NEWPID | CLONE_NEWNET | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_NEWNS | CLONE_NEWUSER)
>>>  
>>> @@ -384,6 +385,50 @@ static int prepare_pstree_for_shell_job(void)
>>>  	return 0;
>>>  }
>>>  
>>> +static struct pid *insert_pid(pid_t pid, struct pid *pid_node)
>>
>> Why struct pid's value is not enough and we have to pass pid_t pid here?
> 
> pid_node is used to insert a thread entry

Huh? Let's check, see below:

>>
>>> +{
>>> +	struct rb_node *node = root_rb.rb_node;
>>> +	struct rb_node **new = &root_rb.rb_node;
>>> +	struct rb_node *parent = NULL;
>>> +
>>> +	while (node) {
>>> +		struct pid *this = rb_entry(node, struct pid, node);
>>> +
>>> +		parent = *new;
>>> +		if (pid < this->virt)
>>> +			node = node->rb_left, new = &((*new)->rb_left);
>>> +		else if (pid > this->virt)
>>> +			node = node->rb_right, new = &((*new)->rb_right);
>>> +		else
>>> +			return this;
>>> +	}

If we get here the node is NULL, correct?

>>> +
>>> +	if (!node) {

And we dive into this if branch.

>>> +		struct pstree_item *item;
>>> +
>>> +		item = alloc_pstree_item_with_rst();
>>> +		if (item == NULL)
>>> +			return NULL;
>>> +
>>> +		item->pid.virt = pid;
>>> +		pid_node = &item->pid;

Here we override whatever value is in the pid_node.

>>> +	}
>>> +	rb_link_and_balance(&root_rb, &pid_node->node, parent, new);
>>> +	return pid_node;

And return it. So what's the point in two args for this function then?

>>> +}
>>> +
>>> +struct pstree_item *insert_item(pid_t pid)
>>
>> This is search_item, rather than insert_ isn't it?
> 
> Its inserts a new entry, if it isn't there yet.

OK, so this is neither pure insert nor pure search. Please, find some
appropriate name.

> We call this function for pid, sid, pgid.

Even sids?!

>>
>>> +{
>>> +	struct pid *node;;
>>> +
>>> +	node = insert_pid(pid, NULL);
>>> +	if (!node)
>>> +		return NULL;
>>> +	BUG_ON(node->state == TASK_THREAD);
>>> +
>>> +	return container_of(node, struct pstree_item, pid);
>>> +}
>>> +
>>>  static int read_pstree_image(void)
>>>  {
>>>  	int ret = 0, i;
>>> @@ -404,18 +449,22 @@ static int read_pstree_image(void)
>>>  			break;
>>>  
>>>  		ret = -1;
>>> -		pi = alloc_pstree_item_with_rst();
>>> +		pi = insert_item(e->pid);
>>>  		if (pi == NULL)
>>>  			break;
>>>  
>>> +		if (insert_item(e->pgid) == NULL)
>>> +			break;
>>> +		if (insert_item(e->sid) == NULL)
>>> +			break;
>>> +
>>
>> Please, explain this.
> 
> * We need to know about all used pids to allocate pids for helpers.
> * Actually we will neet to add helpers for these pids too to restore
>   these sid and pgid.

If we insert an item for e->sid, would the corresponding pstree_item (if
it exists) pick one up?

>>
>>>  		pi->pid.virt = e->pid;
>>>  		max_pid = max((int)e->pid, max_pid);
>>> -
>>>  		pi->pgid = e->pgid;
>>>  		max_pid = max((int)e->pgid, max_pid);
>>> -
>>>  		pi->sid = e->sid;
>>>  		max_pid = max((int)e->sid, max_pid);
>>> +		pi->pid.state = TASK_ALIVE;
>>>  
>>>  		if (e->ppid == 0) {
>>>  			if (root_item) {
>>> @@ -465,7 +514,8 @@ static int read_pstree_image(void)
>>>  		for (i = 0; i < e->n_threads; i++) {
>>>  			pi->threads[i].real = -1;
>>>  			pi->threads[i].virt = e->threads[i];
>>> -			max_pid = max((int)e->threads[i], max_pid);
>>> +			pi->threads[i].state = TASK_THREAD;
>>> +			insert_pid(pi->threads[i].virt, &pi->threads[i]);
>>>  		}
>>>  
>>>  		task_entries->nr_threads += e->n_threads;
>>>
>>
> .
> 



More information about the CRIU mailing list