[CRIU] [PATCH 06/19] pstree: Bind CoreEntry to pstree item

[Cyrill Gorcunov]

On Wed, Feb 27, 2013 at 03:55:17PM +0400, Pavel Emelyanov wrote:
> On 02/26/2013 05:08 PM, Cyrill Gorcunov wrote:
> > 
> > In future we will need to gather task registers pretty early which
> > implies preliminary allocation of the CoreEntry entries. So we bind
> > CoreEntry array to pstree item and allocate/free it with pstree
> > lifetime.
> > 
> > (This is because when parasite daemon mode will be implemented we
> >  no longer able to fetch registers, since task will be running in
> >  daemon code).
> 
> We don't fetch registers for master thread even now:
> 
> int get_task_regs(pid_t pid, CoreEntry *core, const struct parasite_ctl *ctl)
> {
>         struct xsave_struct xsave       = {  };
>         user_regs_struct_t regs         = {-1};
>         
>         struct iovec iov;
>         int ret = -1;
>         
>         pr_info("Dumping GP/FPU registers ... ");
> 
>         if (ctl)
>                 regs = ctl->regs_orig;
>         else {  
>                 if (ptrace(PTRACE_GETREGS, pid, NULL, &regs)) {
>                         pr_err("Can't obtain GP registers for %d\n", pid);
>                         goto err;
>                 }
>         }
> 
> why not simply pull threads regs in the same manner?

Could you please enlight me here? We do (with these patches applied)

static int collect_regs_seized(struct pstree_item *item)
{
	unsigned int i;
	int ret;

	for (i = 0; i < item->nr_threads; i++) {
		pid_t pid = item->threads[i].real;
		ret = get_task_regs(pid, item->core[i], NULL);
		if (ret) {
			pr_err("Can't obtain regs for thread %d\n", pid);
			return -1;
		}
	}

	return 0;
}

here get_task_regs called with NULL and original registers for all
threads (including thread leader) is fetched. I didn't drop ctl
argument since I don't know if we might need it in future. IOW
registers for all threads in pstree item are fetched once item
is seized. No?

> > +	BUG_ON(!core);
> 
> unneeded

OK

> >  
> > +	if (pstree_alloc_cores(item))
> > +		goto err_close;
> 
> Why not allocating cores at the same place where the respective pstree item is?

pstree items are used in both -- checkpoint and restore stages, while for
checkpoint stages CoreEntry will be allocated, they are not needed on restore
moment, so I tried to make code somehow more separated. Still, indeed I'll
review if I can allocate cores together with pstree item creation, thanks!

> > @@ -1147,7 +1106,8 @@ static int dump_task_threads(struct parasite_ctl *parasite_ctl,
> >  			continue;
> >  		}
> >  
> > -		if (dump_task_thread(parasite_ctl, &item->threads[i]))
> > +		if (dump_task_thread(parasite_ctl, &item->threads[i],
> > +				     item->core[i]))
> >  			return -1;
> >  	}
> >  
> > @@ -1354,7 +1314,7 @@ static int dump_one_task(struct pstree_item *item)
> >  		goto err_cure;
> >  	}
> >  
> > -	ret = dump_task_core_all(pid, &pps_buf, &misc,
> > +	ret = dump_task_core_all(pid, pstree_find_core(item, pid), &pps_buf, &misc,
> 
> Linear search is bad here. The pstree_item can point directly to the desired core.

Sure, will update, thanks.

> > +int pstree_alloc_cores(struct pstree_item *item)
> > +{
> > +	unsigned int i;
> > +	bool is_alive;
> > +
> > +	is_alive = (item->state != TASK_DEAD);
> > +	BUG_ON(!is_alive && item->nr_threads > 1);
> 
> If we allocate cores together with pstree-items this bug-on becomes excessive

ok, thanks!

	Cyrill