[CRIU] [PATCH 4/5] pstree: try to find a free pid between busy pids

[Andrew Vagin]

On Thu, Mar 10, 2016 at 11:45:32AM +0300, Pavel Emelyanov wrote:
> On 02/19/2016 09:13 PM, Andrey Vagin wrote:
> > From: Andrew Vagin <avagin at virtuozzo.com>
> > 
> > Currently our pid allocator returns max_pid++ and it can return a pid
> > which is bigger than kernel.max_pid.
> > 
> > (00.821430)   5506: Error (cr-restore.c:1540): Pid 300 do not match expected 32768
> > 
> > https://jira.sw.ru/browse/PSBM-42202
> > Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
> > ---
> >  criu/pstree.c | 93 +++++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 56 insertions(+), 37 deletions(-)
> > 
> > diff --git a/criu/pstree.c b/criu/pstree.c
> > index 1be8c48..61fd5d3 100644
> > --- a/criu/pstree.c
> > +++ b/criu/pstree.c
> > @@ -331,8 +331,6 @@ err:
> >  	return ret;
> >  }
> >  
> > -static int max_pid = 0;
> > -
> >  static int prepare_pstree_for_shell_job(void)
> >  {
> >  	pid_t current_sid = getsid(getpid());
> > @@ -379,8 +377,8 @@ static int prepare_pstree_for_shell_job(void)
> >  			pi->sid = current_sid;
> >  	}
> >  
> > -	max_pid = max((int)current_sid, max_pid);
> > -	max_pid = max((int)current_gid, max_pid);
> > +	insert_item(current_sid);
> > +	insert_item(current_gid);
> 
> insert_item may fail.
> 
> >  
> >  	return 0;
> >  }
> > @@ -459,11 +457,8 @@ static int read_pstree_image(void)
> >  			break;
> >  
> >  		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) {
> > @@ -543,11 +538,34 @@ static int read_pstree_image(void)
> >  				goto err;
> >  		}
> >  	}
> > +
> >  err:
> >  	close_image(img);
> >  	return ret;
> >  }
> >  
> > +#define RESERVED_PIDS           300
> > +static int get_free_pid()
> > +{
> > +	static struct pstree_item *prev, *next;
> > +
> > +	if (prev == NULL)
> > +		prev = rb_entry(rb_first(&root_rb), struct pstree_item, pid.node);
> > +
> > +	while (1) {
> > +		pid_t pid;
> > +		pid = prev->pid.virt + 1;
> > +		pid = pid < RESERVED_PIDS ? RESERVED_PIDS + 1 : pid;
> > +
> > +		next = rb_entry(rb_next(&prev->pid.node), struct pstree_item, pid.node);
> > +		if (&next->pid.node == NULL || next->pid.virt > pid)
> > +			return pid;
> > +		prev = next;
> > +	}
> > +
> > +	return -1;
> > +}
> > +
> >  static int prepare_pstree_ids(void)
> >  {
> >  	struct pstree_item *item, *child, *helper, *tmp;
> > @@ -562,6 +580,7 @@ static int prepare_pstree_ids(void)
> >  	 * reparented to init.
> >  	 */
> >  	list_for_each_entry(item, &root_item->children, sibling) {
> > +		struct pstree_item *leader;
> >  
> >  		/*
> >  		 * If a child belongs to the root task's session or it's
> > @@ -571,15 +590,36 @@ static int prepare_pstree_ids(void)
> >  		if (item->sid == root_item->sid || item->sid == item->pid.virt)
> >  			continue;
> >  
> > -		helper = alloc_pstree_helper();
> > -		if (helper == NULL)
> > -			return -1;
> > -		helper->sid = item->sid;
> > -		helper->pgid = item->sid;
> > -		helper->pid.virt = item->sid;
> > -		helper->parent = root_item;
> > -		helper->ids = root_item->ids;
> > -		list_add_tail(&helper->sibling, &helpers);
> > +		leader = insert_item(item->sid);
> > +		if (leader->pid.state != TASK_UNDEF) {
> 
> This is changed behavior -- before the patch helper was allocated unconditionally,
> now we _may_ switch to some existing task. Explain this change, please.

If you look at old code, you can find that we add a helper here
uncoditionaly, but then we try to find a task with this sid and if it
exists, we connect this helper to this task.

So I don't change a logic here, just move code from one place to another

> 
> > +			pid_t pid;
> > +
> > +			pid = get_free_pid();
> > +			if (pid < 0)
> > +				break;
> > +			helper = insert_item(pid);
> > +			if (helper == NULL)
> > +				return -1;
> > +
> > +			pr_info("Session leader %d\n", item->sid);
> > +
> > +			helper->sid = item->sid;
> > +			helper->pgid = leader->pgid;
> > +			helper->ids = leader->ids;
> > +			helper->parent = leader;
> > +			list_add(&helper->sibling, &leader->children);
> > +
> > +			pr_info("Attach %d to the task %d\n",
> > +					helper->pid.virt, leader->pid.virt);
> > +		} else {
> > +			helper = leader;
> > +			helper->sid = item->sid;
> > +			helper->pgid = item->sid;
> > +			helper->parent = root_item;
> > +			helper->ids = root_item->ids;
> > +			list_add_tail(&helper->sibling, &helpers);
> > +		}
> > +		init_pstree_helper(helper);
> >  
> >  		pr_info("Add a helper %d for restoring SID %d\n",
> >  				helper->pid.virt, helper->sid);
> > @@ -639,27 +679,6 @@ static int prepare_pstree_ids(void)
> >  
> >  			continue;
> >  		}
> > -
> > -		pr_info("Session leader %d\n", item->sid);
> > -
> > -		/* Try to find helpers, who should be connected to the leader */
> > -		list_for_each_entry(child, &helpers, sibling) {
> > -			if (child->pid.state != TASK_HELPER)
> > -				continue;
> > -
> > -			if (child->sid != item->sid)
> > -				continue;
> > -
> > -			child->pgid = item->pgid;
> > -			child->pid.virt = ++max_pid;
> > -			child->parent = item;
> > -			list_move(&child->sibling, &item->children);
> > -
> > -			pr_info("Attach %d to the task %d\n",
> > -					child->pid.virt, item->pid.virt);
> > -
> > -			break;
> > -		}
> >  	}
> >  
> >  	/* All other helpers are session leaders for own sessions */
> > 
>