pstree/pidns: initialize tid->ns[i].virt for threads

Submitted by Pavel Tikhomirov on May 9, 2017, 11:44 a.m.

Details

Message ID 20170509114447.25321-1-ptikhomirov@virtuozzo.com
State Accepted
Series "pstree/pidns: initialize tid->ns[i].virt for threads"
Headers show

Commit Message

Pavel Tikhomirov May 9, 2017, 11:44 a.m.
That is for if-unset check in dump_task_thread(), which compares virt
and -1. It is ok not to initialize virt if kernel has NSpid in
/proc/pid/status as parse_pid_status() will rewrite zeroes, but on
VZ7 kernel it will fail:

https://ci.openvz.org/job/CRIU/job/CRIU-virtuozzo/job/criu-dev/2021/

Patch hide | download patch | download mbox

======================== Run zdtm/static/clone_fs in h =========================
Start test
./clone_fs --pidfile=clone_fs.pid --outfile=clone_fs.out
Run criu dump
=[log]=> dump/zdtm/static/clone_fs/24/1/dump.log
------------------------ grep Error ------------------------
(00.007511) Dumping general registers for 25 in native mode
(00.007525) Dumping GP/FPU registers for 25
(00.007535) 25 has 0 sched policy
(00.007542) 	dumping 0 nice for 25
(00.007549) Error (criu/cr-dump.c:863): Parasite and /proc/[pid]/status gave different tids
------------------------ ERROR OVER ------------------------
Run criu restore
=[log]=> dump/zdtm/static/clone_fs/24/1/restore.log
------------------------ grep Error ------------------------
(00.000497) Add user ns 2 pid 24
(00.000500) Add pid ns 1 pid 24
(00.000503) Add ipc ns 4 pid 24
(00.000506) Add uts ns 5 pid 24
(00.000514) Error (criu/pstree.c:501): Can't skip zero pids levels (0) or find {parent,} ns (1)
(00.000520) Error (criu/pstree.c:813): BUG at criu/pstree.c:813
------------------------ ERROR OVER ------------------------

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/seize.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/criu/seize.c b/criu/seize.c
index 6732151..8d3c6cc 100644
--- a/criu/seize.c
+++ b/criu/seize.c
@@ -688,7 +688,7 @@  static int parse_thread_status(int pid, struct seize_task_status *ss, void *thre
 static int collect_threads(struct pstree_item *item)
 {
 	struct pid **threads = NULL;
-	int nr_threads = 0, i = 0, ret, nr_inprogress, nr_stopped = 0;
+	int nr_threads = 0, i = 0, j, ret, nr_inprogress, nr_stopped = 0;
 	int level = item->pid->level, id;
 
 	ret = parse_threads(item->pid->real, &threads, &nr_threads);
@@ -713,6 +713,8 @@  static int collect_threads(struct pstree_item *item)
 		item->nr_threads = 1;
 		item->threads[0]->item = NULL;
 		item->threads[0]->level = level;
+		for (j = 0; j < level; j++)
+			item->threads[0]->ns[j].virt = -1;
 	}
 
 	nr_inprogress = 0;
@@ -739,6 +741,8 @@  static int collect_threads(struct pstree_item *item)
 		item->threads[id]->real = pid;
 		item->threads[id]->item = NULL;
 		item->threads[id]->level = level;
+		for (j = 0; j < level; j++)
+			item->threads[id]->ns[j].virt = -1;
 
 		ret = compel_wait_task(pid, item_ppid(item), parse_thread_status, NULL,
 				       &t_creds.s, &item->threads[id]);

Comments

Andrey Vagin May 9, 2017, 5:42 p.m.
Kirill, pls review this patch.

On Tue, May 09, 2017 at 02:44:47PM +0300, Pavel Tikhomirov wrote:
> That is for if-unset check in dump_task_thread(), which compares virt
> and -1. It is ok not to initialize virt if kernel has NSpid in
> /proc/pid/status as parse_pid_status() will rewrite zeroes, but on
> VZ7 kernel it will fail:
> 
> https://ci.openvz.org/job/CRIU/job/CRIU-virtuozzo/job/criu-dev/2021/
> 
> ======================== Run zdtm/static/clone_fs in h =========================
> Start test
> ./clone_fs --pidfile=clone_fs.pid --outfile=clone_fs.out
> Run criu dump
> =[log]=> dump/zdtm/static/clone_fs/24/1/dump.log
> ------------------------ grep Error ------------------------
> (00.007511) Dumping general registers for 25 in native mode
> (00.007525) Dumping GP/FPU registers for 25
> (00.007535) 25 has 0 sched policy
> (00.007542) 	dumping 0 nice for 25
> (00.007549) Error (criu/cr-dump.c:863): Parasite and /proc/[pid]/status gave different tids
> ------------------------ ERROR OVER ------------------------
> Run criu restore
> =[log]=> dump/zdtm/static/clone_fs/24/1/restore.log
> ------------------------ grep Error ------------------------
> (00.000497) Add user ns 2 pid 24
> (00.000500) Add pid ns 1 pid 24
> (00.000503) Add ipc ns 4 pid 24
> (00.000506) Add uts ns 5 pid 24
> (00.000514) Error (criu/pstree.c:501): Can't skip zero pids levels (0) or find {parent,} ns (1)
> (00.000520) Error (criu/pstree.c:813): BUG at criu/pstree.c:813
> ------------------------ ERROR OVER ------------------------
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/seize.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/seize.c b/criu/seize.c
> index 6732151..8d3c6cc 100644
> --- a/criu/seize.c
> +++ b/criu/seize.c
> @@ -688,7 +688,7 @@ static int parse_thread_status(int pid, struct seize_task_status *ss, void *thre
>  static int collect_threads(struct pstree_item *item)
>  {
>  	struct pid **threads = NULL;
> -	int nr_threads = 0, i = 0, ret, nr_inprogress, nr_stopped = 0;
> +	int nr_threads = 0, i = 0, j, ret, nr_inprogress, nr_stopped = 0;
>  	int level = item->pid->level, id;
>  
>  	ret = parse_threads(item->pid->real, &threads, &nr_threads);
> @@ -713,6 +713,8 @@ static int collect_threads(struct pstree_item *item)
>  		item->nr_threads = 1;
>  		item->threads[0]->item = NULL;
>  		item->threads[0]->level = level;
> +		for (j = 0; j < level; j++)
> +			item->threads[0]->ns[j].virt = -1;
>  	}
>  
>  	nr_inprogress = 0;
> @@ -739,6 +741,8 @@ static int collect_threads(struct pstree_item *item)
>  		item->threads[id]->real = pid;
>  		item->threads[id]->item = NULL;
>  		item->threads[id]->level = level;
> +		for (j = 0; j < level; j++)
> +			item->threads[id]->ns[j].virt = -1;
>  
>  		ret = compel_wait_task(pid, item_ppid(item), parse_thread_status, NULL,
>  				       &t_creds.s, &item->threads[id]);
> -- 
> 2.9.3
>
Kirill Tkhai May 10, 2017, 8:42 a.m.
On 09.05.2017 14:44, Pavel Tikhomirov wrote:
> That is for if-unset check in dump_task_thread(), which compares virt
> and -1. It is ok not to initialize virt if kernel has NSpid in
> /proc/pid/status as parse_pid_status() will rewrite zeroes, but on
> VZ7 kernel it will fail:
> 
> https://ci.openvz.org/job/CRIU/job/CRIU-virtuozzo/job/criu-dev/2021/
> 
> ======================== Run zdtm/static/clone_fs in h =========================
> Start test
> ./clone_fs --pidfile=clone_fs.pid --outfile=clone_fs.out
> Run criu dump
> =[log]=> dump/zdtm/static/clone_fs/24/1/dump.log
> ------------------------ grep Error ------------------------
> (00.007511) Dumping general registers for 25 in native mode
> (00.007525) Dumping GP/FPU registers for 25
> (00.007535) 25 has 0 sched policy
> (00.007542) 	dumping 0 nice for 25
> (00.007549) Error (criu/cr-dump.c:863): Parasite and /proc/[pid]/status gave different tids
> ------------------------ ERROR OVER ------------------------
> Run criu restore
> =[log]=> dump/zdtm/static/clone_fs/24/1/restore.log
> ------------------------ grep Error ------------------------
> (00.000497) Add user ns 2 pid 24
> (00.000500) Add pid ns 1 pid 24
> (00.000503) Add ipc ns 4 pid 24
> (00.000506) Add uts ns 5 pid 24
> (00.000514) Error (criu/pstree.c:501): Can't skip zero pids levels (0) or find {parent,} ns (1)
> (00.000520) Error (criu/pstree.c:813): BUG at criu/pstree.c:813
> ------------------------ ERROR OVER ------------------------
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

Thanks!

Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  criu/seize.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/seize.c b/criu/seize.c
> index 6732151..8d3c6cc 100644
> --- a/criu/seize.c
> +++ b/criu/seize.c
> @@ -688,7 +688,7 @@ static int parse_thread_status(int pid, struct seize_task_status *ss, void *thre
>  static int collect_threads(struct pstree_item *item)
>  {
>  	struct pid **threads = NULL;
> -	int nr_threads = 0, i = 0, ret, nr_inprogress, nr_stopped = 0;
> +	int nr_threads = 0, i = 0, j, ret, nr_inprogress, nr_stopped = 0;
>  	int level = item->pid->level, id;
>  
>  	ret = parse_threads(item->pid->real, &threads, &nr_threads);
> @@ -713,6 +713,8 @@ static int collect_threads(struct pstree_item *item)
>  		item->nr_threads = 1;
>  		item->threads[0]->item = NULL;
>  		item->threads[0]->level = level;
> +		for (j = 0; j < level; j++)
> +			item->threads[0]->ns[j].virt = -1;
>  	}
>  
>  	nr_inprogress = 0;
> @@ -739,6 +741,8 @@ static int collect_threads(struct pstree_item *item)
>  		item->threads[id]->real = pid;
>  		item->threads[id]->item = NULL;
>  		item->threads[id]->level = level;
> +		for (j = 0; j < level; j++)
> +			item->threads[id]->ns[j].virt = -1;
>  
>  		ret = compel_wait_task(pid, item_ppid(item), parse_thread_status, NULL,
>  				       &t_creds.s, &item->threads[id]);
>
Kirill Tkhai May 10, 2017, 12:43 p.m.
On 10.05.2017 11:42, Kirill Tkhai wrote:
> On 09.05.2017 14:44, Pavel Tikhomirov wrote:
>> That is for if-unset check in dump_task_thread(), which compares virt
>> and -1. It is ok not to initialize virt if kernel has NSpid in
>> /proc/pid/status as parse_pid_status() will rewrite zeroes, but on
>> VZ7 kernel it will fail:
>>
>> https://ci.openvz.org/job/CRIU/job/CRIU-virtuozzo/job/criu-dev/2021/
>>
>> ======================== Run zdtm/static/clone_fs in h =========================
>> Start test
>> ./clone_fs --pidfile=clone_fs.pid --outfile=clone_fs.out
>> Run criu dump
>> =[log]=> dump/zdtm/static/clone_fs/24/1/dump.log
>> ------------------------ grep Error ------------------------
>> (00.007511) Dumping general registers for 25 in native mode
>> (00.007525) Dumping GP/FPU registers for 25
>> (00.007535) 25 has 0 sched policy
>> (00.007542) 	dumping 0 nice for 25
>> (00.007549) Error (criu/cr-dump.c:863): Parasite and /proc/[pid]/status gave different tids
>> ------------------------ ERROR OVER ------------------------
>> Run criu restore
>> =[log]=> dump/zdtm/static/clone_fs/24/1/restore.log
>> ------------------------ grep Error ------------------------
>> (00.000497) Add user ns 2 pid 24
>> (00.000500) Add pid ns 1 pid 24
>> (00.000503) Add ipc ns 4 pid 24
>> (00.000506) Add uts ns 5 pid 24
>> (00.000514) Error (criu/pstree.c:501): Can't skip zero pids levels (0) or find {parent,} ns (1)
>> (00.000520) Error (criu/pstree.c:813): BUG at criu/pstree.c:813
>> ------------------------ ERROR OVER ------------------------
>>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> 
> Thanks!
> 
> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> 
>> ---
>>  criu/seize.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/criu/seize.c b/criu/seize.c
>> index 6732151..8d3c6cc 100644
>> --- a/criu/seize.c
>> +++ b/criu/seize.c
>> @@ -688,7 +688,7 @@ static int parse_thread_status(int pid, struct seize_task_status *ss, void *thre
>>  static int collect_threads(struct pstree_item *item)
>>  {
>>  	struct pid **threads = NULL;
>> -	int nr_threads = 0, i = 0, ret, nr_inprogress, nr_stopped = 0;
>> +	int nr_threads = 0, i = 0, j, ret, nr_inprogress, nr_stopped = 0;
>>  	int level = item->pid->level, id;
>>  
>>  	ret = parse_threads(item->pid->real, &threads, &nr_threads);
>> @@ -713,6 +713,8 @@ static int collect_threads(struct pstree_item *item)
>>  		item->nr_threads = 1;
>>  		item->threads[0]->item = NULL;
>>  		item->threads[0]->level = level;
>> +		for (j = 0; j < level; j++)
>> +			item->threads[0]->ns[j].virt = -1;

We do not need cycle here and below

>>  	}
>>  
>>  	nr_inprogress = 0;
>> @@ -739,6 +741,8 @@ static int collect_threads(struct pstree_item *item)
>>  		item->threads[id]->real = pid;
>>  		item->threads[id]->item = NULL;
>>  		item->threads[id]->level = level;
>> +		for (j = 0; j < level; j++)
>> +			item->threads[id]->ns[j].virt = -1;
>>  
>>  		ret = compel_wait_task(pid, item_ppid(item), parse_thread_status, NULL,
>>  				       &t_creds.s, &item->threads[id]);
>>
Kirill Tkhai May 10, 2017, 12:46 p.m.
On 10.05.2017 15:43, Kirill Tkhai wrote:
> On 10.05.2017 11:42, Kirill Tkhai wrote:
>> On 09.05.2017 14:44, Pavel Tikhomirov wrote:
>>> That is for if-unset check in dump_task_thread(), which compares virt
>>> and -1. It is ok not to initialize virt if kernel has NSpid in
>>> /proc/pid/status as parse_pid_status() will rewrite zeroes, but on
>>> VZ7 kernel it will fail:
>>>
>>> https://ci.openvz.org/job/CRIU/job/CRIU-virtuozzo/job/criu-dev/2021/
>>>
>>> ======================== Run zdtm/static/clone_fs in h =========================
>>> Start test
>>> ./clone_fs --pidfile=clone_fs.pid --outfile=clone_fs.out
>>> Run criu dump
>>> =[log]=> dump/zdtm/static/clone_fs/24/1/dump.log
>>> ------------------------ grep Error ------------------------
>>> (00.007511) Dumping general registers for 25 in native mode
>>> (00.007525) Dumping GP/FPU registers for 25
>>> (00.007535) 25 has 0 sched policy
>>> (00.007542) 	dumping 0 nice for 25
>>> (00.007549) Error (criu/cr-dump.c:863): Parasite and /proc/[pid]/status gave different tids
>>> ------------------------ ERROR OVER ------------------------
>>> Run criu restore
>>> =[log]=> dump/zdtm/static/clone_fs/24/1/restore.log
>>> ------------------------ grep Error ------------------------
>>> (00.000497) Add user ns 2 pid 24
>>> (00.000500) Add pid ns 1 pid 24
>>> (00.000503) Add ipc ns 4 pid 24
>>> (00.000506) Add uts ns 5 pid 24
>>> (00.000514) Error (criu/pstree.c:501): Can't skip zero pids levels (0) or find {parent,} ns (1)
>>> (00.000520) Error (criu/pstree.c:813): BUG at criu/pstree.c:813
>>> ------------------------ ERROR OVER ------------------------
>>>
>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>
>> Thanks!
>>
>> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>
>>> ---
>>>  criu/seize.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/criu/seize.c b/criu/seize.c
>>> index 6732151..8d3c6cc 100644
>>> --- a/criu/seize.c
>>> +++ b/criu/seize.c
>>> @@ -688,7 +688,7 @@ static int parse_thread_status(int pid, struct seize_task_status *ss, void *thre
>>>  static int collect_threads(struct pstree_item *item)
>>>  {
>>>  	struct pid **threads = NULL;
>>> -	int nr_threads = 0, i = 0, ret, nr_inprogress, nr_stopped = 0;
>>> +	int nr_threads = 0, i = 0, j, ret, nr_inprogress, nr_stopped = 0;
>>>  	int level = item->pid->level, id;
>>>  
>>>  	ret = parse_threads(item->pid->real, &threads, &nr_threads);
>>> @@ -713,6 +713,8 @@ static int collect_threads(struct pstree_item *item)
>>>  		item->nr_threads = 1;
>>>  		item->threads[0]->item = NULL;
>>>  		item->threads[0]->level = level;
>>> +		for (j = 0; j < level; j++)
>>> +			item->threads[0]->ns[j].virt = -1;
> 
> We do not need cycle here and below

But, OK, lets leave it here just for uniformity.
 
>>>  	}
>>>  
>>>  	nr_inprogress = 0;
>>> @@ -739,6 +741,8 @@ static int collect_threads(struct pstree_item *item)
>>>  		item->threads[id]->real = pid;
>>>  		item->threads[id]->item = NULL;
>>>  		item->threads[id]->level = level;
>>> +		for (j = 0; j < level; j++)
>>> +			item->threads[id]->ns[j].virt = -1;
>>>  
>>>  		ret = compel_wait_task(pid, item_ppid(item), parse_thread_status, NULL,
>>>  				       &t_creds.s, &item->threads[id]);
>>>
Andrey Vagin May 11, 2017, 12:49 a.m.
Applied, thanks!

On Wed, May 10, 2017 at 11:42:06AM +0300, Kirill Tkhai wrote:
> On 09.05.2017 14:44, Pavel Tikhomirov wrote:
> > That is for if-unset check in dump_task_thread(), which compares virt
> > and -1. It is ok not to initialize virt if kernel has NSpid in
> > /proc/pid/status as parse_pid_status() will rewrite zeroes, but on
> > VZ7 kernel it will fail:
> > 
> > https://ci.openvz.org/job/CRIU/job/CRIU-virtuozzo/job/criu-dev/2021/
> > 
> > ======================== Run zdtm/static/clone_fs in h =========================
> > Start test
> > ./clone_fs --pidfile=clone_fs.pid --outfile=clone_fs.out
> > Run criu dump
> > =[log]=> dump/zdtm/static/clone_fs/24/1/dump.log
> > ------------------------ grep Error ------------------------
> > (00.007511) Dumping general registers for 25 in native mode
> > (00.007525) Dumping GP/FPU registers for 25
> > (00.007535) 25 has 0 sched policy
> > (00.007542) 	dumping 0 nice for 25
> > (00.007549) Error (criu/cr-dump.c:863): Parasite and /proc/[pid]/status gave different tids
> > ------------------------ ERROR OVER ------------------------
> > Run criu restore
> > =[log]=> dump/zdtm/static/clone_fs/24/1/restore.log
> > ------------------------ grep Error ------------------------
> > (00.000497) Add user ns 2 pid 24
> > (00.000500) Add pid ns 1 pid 24
> > (00.000503) Add ipc ns 4 pid 24
> > (00.000506) Add uts ns 5 pid 24
> > (00.000514) Error (criu/pstree.c:501): Can't skip zero pids levels (0) or find {parent,} ns (1)
> > (00.000520) Error (criu/pstree.c:813): BUG at criu/pstree.c:813
> > ------------------------ ERROR OVER ------------------------
> > 
> > Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> 
> Thanks!
> 
> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> 
> > ---
> >  criu/seize.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/criu/seize.c b/criu/seize.c
> > index 6732151..8d3c6cc 100644
> > --- a/criu/seize.c
> > +++ b/criu/seize.c
> > @@ -688,7 +688,7 @@ static int parse_thread_status(int pid, struct seize_task_status *ss, void *thre
> >  static int collect_threads(struct pstree_item *item)
> >  {
> >  	struct pid **threads = NULL;
> > -	int nr_threads = 0, i = 0, ret, nr_inprogress, nr_stopped = 0;
> > +	int nr_threads = 0, i = 0, j, ret, nr_inprogress, nr_stopped = 0;
> >  	int level = item->pid->level, id;
> >  
> >  	ret = parse_threads(item->pid->real, &threads, &nr_threads);
> > @@ -713,6 +713,8 @@ static int collect_threads(struct pstree_item *item)
> >  		item->nr_threads = 1;
> >  		item->threads[0]->item = NULL;
> >  		item->threads[0]->level = level;
> > +		for (j = 0; j < level; j++)
> > +			item->threads[0]->ns[j].virt = -1;
> >  	}
> >  
> >  	nr_inprogress = 0;
> > @@ -739,6 +741,8 @@ static int collect_threads(struct pstree_item *item)
> >  		item->threads[id]->real = pid;
> >  		item->threads[id]->item = NULL;
> >  		item->threads[id]->level = level;
> > +		for (j = 0; j < level; j++)
> > +			item->threads[id]->ns[j].virt = -1;
> >  
> >  		ret = compel_wait_task(pid, item_ppid(item), parse_thread_status, NULL,
> >  				       &t_creds.s, &item->threads[id]);
> >