GSoC: Memory mapping collection

Submitted by Abhishek Dubey on June 4, 2019, 4:50 p.m.

Details

Message ID CAODppzH5V4YT0n-MPBm_Amd-x9eTD9Za48KeagoxzzG2za9vBA@mail.gmail.com
State New
Series "GSoC: Memory mapping collection"
Headers show

Commit Message

Abhishek Dubey June 4, 2019, 4:50 p.m.
Hi,

On running "zdtm.py run -a", testcases of zdtm/static directory are checked
and that of zdtm/transition are skipped. Do all testcases of transition
directory need manual run?
What is specific about transition testcases?

For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
- To maintain correspondence between PID and collected VMA, added new entry
in struct pstree_item
- Moved out unfreeze code from cr_pre_dump_finish()
- Moved out collect_mappings() from pre_dump_one_task

I have tested these changes with env00-basic example and zdtm.py
test-suite. All test-cases under zdtm/static directory are Passing.
Remaining transition test cases are skipped.

Share your thoughts on this approach. I am attaching patch-file for
reference.

-Abhishek

Patch hide | download patch | download mbox

From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001
From: abhidubey4178 <abhishekdubey4178940@gmail.com>
Date: Tue, 4 Jun 2019 19:59:21 +0530
Subject: [PATCH] freezing target task to collect memory mappings, followed by
 unfreeze

---
 criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
 criu/include/pstree.h |   1 +
 2 files changed, 78 insertions(+), 26 deletions(-)

diff --git a/criu/cr-dump.c b/criu/cr-dump.c
index 7f2e5edf..40bc7594 100644
--- a/criu/cr-dump.c
+++ b/criu/cr-dump.c
@@ -1154,20 +1154,19 @@  static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
 	if (item->pid->state == TASK_DEAD)
 		return 0;
 
-	ret = collect_mappings(pid, &vmas, NULL);
-	if (ret) {
+	if (!item->vmas) {
 		pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
 		goto err;
 	}
 
 	ret = -1;
-	parasite_ctl = parasite_infect_seized(pid, item, &vmas);
+	parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
 	if (!parasite_ctl) {
 		pr_err("Can't infect (pid: %d) with parasite\n", pid);
 		goto err_free;
 	}
 
-	ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
+	ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
 	if (ret) {
 		pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
 		goto err_cure;
@@ -1192,14 +1191,14 @@  static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
 	mdc.stat = NULL;
 	mdc.parent_ie = parent_ie;
 
-	ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
+	ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
 	if (ret)
 		goto err_cure;
 
 	if (compel_cure_remote(parasite_ctl))
 		pr_err("Can't cure (pid: %d) from parasite\n", pid);
 err_free:
-	free_mappings(&vmas);
+	free_mappings(item->vmas);
 err:
 	return ret;
 
@@ -1471,26 +1470,9 @@  static int setup_alarm_handler()
 
 static int cr_pre_dump_finish(int status)
 {
-	InventoryEntry he = INVENTORY_ENTRY__INIT;
 	struct pstree_item *item;
 	int ret;
 
-	/*
-	 * Restore registers for tasks only. The threads have not been
-	 * infected. Therefore, the thread register sets have not been changed.
-	 */
-	ret = arch_set_thread_regs(root_item, false);
-	if (ret)
-		goto err;
-
-	ret = inventory_save_uptime(&he);
-	if (ret)
-		goto err;
-
-	pstree_switch_state(root_item, TASK_ALIVE);
-
-	timing_stop(TIME_FROZEN);
-
 	if (status < 0) {
 		ret = status;
 		goto err;
@@ -1540,9 +1522,6 @@  err:
 	if (bfd_flush_images())
 		ret = -1;
 
-	if (write_img_inventory(&he))
-		ret = -1;
-
 	if (ret)
 		pr_err("Pre-dumping FAILED.\n");
 	else {
@@ -1552,6 +1531,68 @@  err:
 	return ret;
 }
 
+
+static int pre_dump_collect_vmas(struct pstree_item *item)
+{
+       pid_t pid = item->pid->real;
+       struct vm_area_list *vmas;
+       int ret = -1;
+
+       vmas = xmalloc(sizeof(struct vm_area_list));
+       if(!vmas)
+            return -1;
+       INIT_LIST_HEAD(&vmas->h);
+       vmas->nr = 0;
+
+       pr_info("========================================\n");
+       pr_info("Collecting VMAs of (pid: %d)\n", pid);
+       pr_info("========================================\n");
+
+       if (item->pid->state == TASK_STOPPED) {
+               pr_warn("Stopped tasks are not supported\n");
+                    return 0;
+            }
+
+       if (item->pid->state == TASK_DEAD)
+                    return 0;
+
+       ret = collect_mappings(pid, vmas, NULL);
+       if (ret) {
+               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
+               return -1;
+            }
+
+       item->vmas = vmas;
+       return 0;
+}
+
+
+static int unfreeze_pstree()
+{
+	InventoryEntry he = INVENTORY_ENTRY__INIT;
+	int ret;
+
+	/*
+	 * Restore registers for tasks only. The threads have not been
+	 * infected. Therefore, the thread register sets have not been changed.
+	 */
+	ret = arch_set_thread_regs(root_item, false);
+	if (ret)
+		return -1;
+
+	ret = inventory_save_uptime(&he);
+	if (ret)
+		return -1;
+
+	if (write_img_inventory(&he))
+		return -1;
+
+	pstree_switch_state(root_item, TASK_ALIVE);
+	timing_stop(TIME_FROZEN);
+
+	return 0;
+}
+
 int cr_pre_dump_tasks(pid_t pid)
 {
 	InventoryEntry *parent_ie = NULL;
@@ -1619,6 +1660,16 @@  int cr_pre_dump_tasks(pid_t pid)
 	/* Errors handled later in detect_pid_reuse */
 	parent_ie = get_parent_inventory();
 
+	/* Collecting VMAs of all processes in pstree */
+	for_each_pstree_item(item)
+		if (pre_dump_collect_vmas(item))
+			goto err;
+
+	/* Unfreeze pstree */
+	ret = unfreeze_pstree();
+	if(ret)
+		goto err;
+
 	for_each_pstree_item(item)
 		if (pre_dump_one_task(item, parent_ie))
 			goto err;
diff --git a/criu/include/pstree.h b/criu/include/pstree.h
index 7303c1fe..69562608 100644
--- a/criu/include/pstree.h
+++ b/criu/include/pstree.h
@@ -30,6 +30,7 @@  struct pstree_item {
 		futex_t		task_st;
 		unsigned long	task_st_le_bits;
 	};
+	struct vm_area_list *vmas;
 };
 
 static inline pid_t vpid(const struct pstree_item *i)
-- 
2.17.1


Comments

Pavel Emelianov June 5, 2019, 12:49 p.m.
On 6/4/19 7:50 PM, Abhishek Dubey wrote:
> Hi,
> 
> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?

Yes.

> What is specific about transition testcases?

They take time to accomplish, so the default run is limited.

> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
> - Moved out unfreeze code from cr_pre_dump_finish()
> - Moved out collect_mappings() from pre_dump_one_task
> 
> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
> 
> Share your thoughts on this approach. I am attaching patch-file for reference.

> 
> Hi,
> 
> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> What is specific about transition testcases?
> 
> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
> - Moved out unfreeze code from cr_pre_dump_finish()
> - Moved out collect_mappings() from pre_dump_one_task

Please, describe why you need this. Current code does "fetching vmas between freeze and unfreeze", 
what's wrong with it so you need this change?

> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
> 
> Share your thoughts on this approach. I am attaching patch-file for reference.
> 
> -Abhishek
> 
> 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
> 
> From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001

Please mind the https://criu.org/How_to_submit_patches article. Patch should go inline, not attached,
and properly formatted. Also comments are inline.

> From: abhidubey4178 <abhishekdubey4178940@gmail.com>
> Date: Tue, 4 Jun 2019 19:59:21 +0530
> Subject: [PATCH] freezing target task to collect memory mappings, followed by
>  unfreeze
> 
> ---
>  criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
>  criu/include/pstree.h |   1 +
>  2 files changed, 78 insertions(+), 26 deletions(-)
> 
> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
> index 7f2e5edf..40bc7594 100644
> --- a/criu/cr-dump.c
> +++ b/criu/cr-dump.c
> @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>  	if (item->pid->state == TASK_DEAD)
>  		return 0;
>  
> -	ret = collect_mappings(pid, &vmas, NULL);

Up above there have been left the now unused vmas variable.

> -	if (ret) {
> +	if (!item->vmas) {
>  		pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>  		goto err;
>  	}
>  
>  	ret = -1;
> -	parasite_ctl = parasite_infect_seized(pid, item, &vmas);
> +	parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
>  	if (!parasite_ctl) {
>  		pr_err("Can't infect (pid: %d) with parasite\n", pid);
>  		goto err_free;
>  	}
>  
> -	ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
> +	ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
>  	if (ret) {
>  		pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
>  		goto err_cure;
> @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>  	mdc.stat = NULL;
>  	mdc.parent_ie = parent_ie;
>  
> -	ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
> +	ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
>  	if (ret)
>  		goto err_cure;
>  
>  	if (compel_cure_remote(parasite_ctl))
>  		pr_err("Can't cure (pid: %d) from parasite\n", pid);
>  err_free:
> -	free_mappings(&vmas);
> +	free_mappings(item->vmas);
>  err:
>  	return ret;
>  
> @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
>  
>  static int cr_pre_dump_finish(int status)
>  {
> -	InventoryEntry he = INVENTORY_ENTRY__INIT;
>  	struct pstree_item *item;
>  	int ret;
>  
> -	/*
> -	 * Restore registers for tasks only. The threads have not been
> -	 * infected. Therefore, the thread register sets have not been changed.
> -	 */
> -	ret = arch_set_thread_regs(root_item, false);
> -	if (ret)
> -		goto err;
> -
> -	ret = inventory_save_uptime(&he);
> -	if (ret)
> -		goto err;
> -
> -	pstree_switch_state(root_item, TASK_ALIVE);
> -
> -	timing_stop(TIME_FROZEN);
> -
>  	if (status < 0) {
>  		ret = status;
>  		goto err;
> @@ -1540,9 +1522,6 @@ err:
>  	if (bfd_flush_images())
>  		ret = -1;
>  
> -	if (write_img_inventory(&he))
> -		ret = -1;
> -

You write inventory too early, I'm not 100% sure this is correct.

>  	if (ret)
>  		pr_err("Pre-dumping FAILED.\n");
>  	else {
> @@ -1552,6 +1531,68 @@ err:
>  	return ret;
>  }
>  
> +
> +static int pre_dump_collect_vmas(struct pstree_item *item)
> +{
> +       pid_t pid = item->pid->real;
> +       struct vm_area_list *vmas;
> +       int ret = -1;
> +
> +       vmas = xmalloc(sizeof(struct vm_area_list));

This memory is never free-d.

> +       if(!vmas)
> +            return -1;
> +       INIT_LIST_HEAD(&vmas->h);
> +       vmas->nr = 0;
> +
> +       pr_info("========================================\n");
> +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
> +       pr_info("========================================\n");
> +
> +       if (item->pid->state == TASK_STOPPED) {
> +               pr_warn("Stopped tasks are not supported\n");
> +                    return 0;
> +            }

Formatting.

> +
> +       if (item->pid->state == TASK_DEAD)
> +                    return 0;

Vmas allocation can happen here.

> +
> +       ret = collect_mappings(pid, vmas, NULL);
> +       if (ret) {
> +               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
> +               return -1;
> +            }
> +
> +       item->vmas = vmas;
> +       return 0;
> +}
> +
> +
> +static int unfreeze_pstree()
> +{
> +	InventoryEntry he = INVENTORY_ENTRY__INIT;
> +	int ret;
> +
> +	/*
> +	 * Restore registers for tasks only. The threads have not been
> +	 * infected. Therefore, the thread register sets have not been changed.
> +	 */
> +	ret = arch_set_thread_regs(root_item, false);
> +	if (ret)
> +		return -1;
> +
> +	ret = inventory_save_uptime(&he);
> +	if (ret)
> +		return -1;
> +
> +	if (write_img_inventory(&he))
> +		return -1;
> +
> +	pstree_switch_state(root_item, TASK_ALIVE);
> +	timing_stop(TIME_FROZEN);
> +
> +	return 0;
> +}
> +
>  int cr_pre_dump_tasks(pid_t pid)
>  {
>  	InventoryEntry *parent_ie = NULL;
> @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
>  	/* Errors handled later in detect_pid_reuse */
>  	parent_ie = get_parent_inventory();
>  
> +	/* Collecting VMAs of all processes in pstree */
> +	for_each_pstree_item(item)
> +		if (pre_dump_collect_vmas(item))
> +			goto err;
> +
> +	/* Unfreeze pstree */

This is pointless comment as it 100% repeats the subsequent function name :)

> +	ret = unfreeze_pstree();
> +	if(ret)
> +		goto err;
> +
>  	for_each_pstree_item(item)
>  		if (pre_dump_one_task(item, parent_ie))
>  			goto err;
> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
> index 7303c1fe..69562608 100644
> --- a/criu/include/pstree.h
> +++ b/criu/include/pstree.h
> @@ -30,6 +30,7 @@ struct pstree_item {
>  		futex_t		task_st;
>  		unsigned long	task_st_le_bits;
>  	};
> +	struct vm_area_list *vmas;

This is dump-only field, it should be placed on dmp_info instead.

>  };
>  
>  static inline pid_t vpid(const struct pstree_item *i)
> -- 
> 2.17.1
> 

-- Pavel
Abhishek Dubey June 6, 2019, 7:31 a.m.
Please find Inline reply.
On 05/06/19 6:19 PM, Pavel Emelianov wrote:
> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
>> Hi,
>>
>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> Yes.
>
>> What is specific about transition testcases?
> They take time to accomplish, so the default run is limited.
So checking default is enough for patch submission?
>
>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>> - Moved out unfreeze code from cr_pre_dump_finish()
>> - Moved out collect_mappings() from pre_dump_one_task
>>
>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>
>> Share your thoughts on this approach. I am attaching patch-file for reference.
>> Hi,
>>
>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
>> What is specific about transition testcases?
>>
>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>> - Moved out unfreeze code from cr_pre_dump_finish()
>> - Moved out collect_mappings() from pre_dump_one_task
> Please, describe why you need this. Current code does "fetching vmas between freeze and unfreeze",
> what's wrong with it so you need this change?

In current approach TIME_FROZEN encapsulates TIME_MEMDUMP, like:

*start(TIME_FROZEN)*

*        collect mappings*

*        start(TIME_MEMDUMP)*

*                step0: page map cache init*

*                step1: generate pagemap **
*

*                step2: grab pages into pipes*

*         end(TIME_MEMDUMP)*

*         unfreeze pstree
*

*    end(TIME_FROZEN)*

and the objective I perceived, is something like:

*start(TIME_FROZEN)           ----} ***

*            collect mappings }==> Current patch
*

*            unfreeze pstree             }
*

***end(TIME_FROZEN)            ----}*

*    start(TIME_MEMDUMP) *

*            step0: page map cache init*

**

*            step1: generate pagemap **
*

**

*            step2: grab pages into pipes*

**

*    end(TIME_MEMDUMP)*

*
*

With reference to the comment "fetching vmas between freeze and 
unfreeze", I think this is what is needed:

*    start(TIME_FROZEN)*

*        collect mappings*

*        start(TIME_MEMDUMP)*

*                step0: page map cache init*

*                step1: generate pagemap **
*

*         end(TIME_MEMDUMP)*

*         unfreeze pstree
*

*    end(TIME_FROZEN)*


**    step2: grab pages into user buffer using system_vm_readv and 
resolve races
**

  Please confirm if above design is right?

  So, I don't have to worry about unfreezing after collection? Let it 
happen the way it is now.

My immediate task will be getting pipes out of picture. I will start 
with "commit bb98a82" as reference, as

suggested by Radostin in other thread.

>
>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>
>> Share your thoughts on this approach. I am attaching patch-file for reference.
>>
>> -Abhishek
>>
>> 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
>>
>>  From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001
> Please mind the https://criu.org/How_to_submit_patches article. Patch should go inline, not attached,
> and properly formatted. Also comments are inline.
Sure, I will take care of same when sending upstream patches. This patch 
was just to discuss approach and not meant in any way to be upstream.
>
>> From: abhidubey4178 <abhishekdubey4178940@gmail.com>
>> Date: Tue, 4 Jun 2019 19:59:21 +0530
>> Subject: [PATCH] freezing target task to collect memory mappings, followed by
>>   unfreeze
>>
>> ---
>>   criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
>>   criu/include/pstree.h |   1 +
>>   2 files changed, 78 insertions(+), 26 deletions(-)
>>
>> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
>> index 7f2e5edf..40bc7594 100644
>> --- a/criu/cr-dump.c
>> +++ b/criu/cr-dump.c
>> @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>   	if (item->pid->state == TASK_DEAD)
>>   		return 0;
>>   
>> -	ret = collect_mappings(pid, &vmas, NULL);
> Up above there have been left the now unused vmas variable.
ok.
>
>> -	if (ret) {
>> +	if (!item->vmas) {
>>   		pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>>   		goto err;
>>   	}
>>   
>>   	ret = -1;
>> -	parasite_ctl = parasite_infect_seized(pid, item, &vmas);
>> +	parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
>>   	if (!parasite_ctl) {
>>   		pr_err("Can't infect (pid: %d) with parasite\n", pid);
>>   		goto err_free;
>>   	}
>>   
>> -	ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
>> +	ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
>>   	if (ret) {
>>   		pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
>>   		goto err_cure;
>> @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>   	mdc.stat = NULL;
>>   	mdc.parent_ie = parent_ie;
>>   
>> -	ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
>> +	ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
>>   	if (ret)
>>   		goto err_cure;
>>   
>>   	if (compel_cure_remote(parasite_ctl))
>>   		pr_err("Can't cure (pid: %d) from parasite\n", pid);
>>   err_free:
>> -	free_mappings(&vmas);
>> +	free_mappings(item->vmas);
>>   err:
>>   	return ret;
>>   
>> @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
>>   
>>   static int cr_pre_dump_finish(int status)
>>   {
>> -	InventoryEntry he = INVENTORY_ENTRY__INIT;
>>   	struct pstree_item *item;
>>   	int ret;
>>   
>> -	/*
>> -	 * Restore registers for tasks only. The threads have not been
>> -	 * infected. Therefore, the thread register sets have not been changed.
>> -	 */
>> -	ret = arch_set_thread_regs(root_item, false);
>> -	if (ret)
>> -		goto err;
>> -
>> -	ret = inventory_save_uptime(&he);
>> -	if (ret)
>> -		goto err;
>> -
>> -	pstree_switch_state(root_item, TASK_ALIVE);
>> -
>> -	timing_stop(TIME_FROZEN);
>> -
>>   	if (status < 0) {
>>   		ret = status;
>>   		goto err;
>> @@ -1540,9 +1522,6 @@ err:
>>   	if (bfd_flush_images())
>>   		ret = -1;
>>   
>> -	if (write_img_inventory(&he))
>> -		ret = -1;
>> -
> You write inventory too early, I'm not 100% sure this is correct.
>
>>   	if (ret)
>>   		pr_err("Pre-dumping FAILED.\n");
>>   	else {
>> @@ -1552,6 +1531,68 @@ err:
>>   	return ret;
>>   }
>>   
>> +
>> +static int pre_dump_collect_vmas(struct pstree_item *item)
>> +{
>> +       pid_t pid = item->pid->real;
>> +       struct vm_area_list *vmas;
>> +       int ret = -1;
>> +
>> +       vmas = xmalloc(sizeof(struct vm_area_list));
> This memory is never free-d.
Got it.
>
>> +       if(!vmas)
>> +            return -1;
>> +       INIT_LIST_HEAD(&vmas->h);
>> +       vmas->nr = 0;
>> +
>> +       pr_info("========================================\n");
>> +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
>> +       pr_info("========================================\n");
>> +
>> +       if (item->pid->state == TASK_STOPPED) {
>> +               pr_warn("Stopped tasks are not supported\n");
>> +                    return 0;
>> +            }
> Formatting.
>
>> +
>> +       if (item->pid->state == TASK_DEAD)
>> +                    return 0;
> Vmas allocation can happen here.
Got it.
>
>> +
>> +       ret = collect_mappings(pid, vmas, NULL);
>> +       if (ret) {
>> +               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>> +               return -1;
>> +            }
>> +
>> +       item->vmas = vmas;
>> +       return 0;
>> +}
>> +
>> +
>> +static int unfreeze_pstree()
>> +{
>> +	InventoryEntry he = INVENTORY_ENTRY__INIT;
>> +	int ret;
>> +
>> +	/*
>> +	 * Restore registers for tasks only. The threads have not been
>> +	 * infected. Therefore, the thread register sets have not been changed.
>> +	 */
>> +	ret = arch_set_thread_regs(root_item, false);
>> +	if (ret)
>> +		return -1;
>> +
>> +	ret = inventory_save_uptime(&he);
>> +	if (ret)
>> +		return -1;
>> +
>> +	if (write_img_inventory(&he))
>> +		return -1;
>> +
>> +	pstree_switch_state(root_item, TASK_ALIVE);
>> +	timing_stop(TIME_FROZEN);
>> +
>> +	return 0;
>> +}
>> +
>>   int cr_pre_dump_tasks(pid_t pid)
>>   {
>>   	InventoryEntry *parent_ie = NULL;
>> @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
>>   	/* Errors handled later in detect_pid_reuse */
>>   	parent_ie = get_parent_inventory();
>>   
>> +	/* Collecting VMAs of all processes in pstree */
>> +	for_each_pstree_item(item)
>> +		if (pre_dump_collect_vmas(item))
>> +			goto err;
>> +
>> +	/* Unfreeze pstree */
> This is pointless comment as it 100% repeats the subsequent function name :)
Won't repeat!
>
>> +	ret = unfreeze_pstree();
>> +	if(ret)
>> +		goto err;
>> +
>>   	for_each_pstree_item(item)
>>   		if (pre_dump_one_task(item, parent_ie))
>>   			goto err;
>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
>> index 7303c1fe..69562608 100644
>> --- a/criu/include/pstree.h
>> +++ b/criu/include/pstree.h
>> @@ -30,6 +30,7 @@ struct pstree_item {
>>   		futex_t		task_st;
>>   		unsigned long	task_st_le_bits;
>>   	};
>> +	struct vm_area_list *vmas;
> This is dump-only field, it should be placed on dmp_info instead.
Sure.
>
>>   };
>>   
>>   static inline pid_t vpid(const struct pstree_item *i)
>> -- 
>> 2.17.1
>>
> -- Pavel
Pavel Emelianov June 7, 2019, 3:10 p.m.
On 6/6/19 10:31 AM, abhishek dubey wrote:
> Please find Inline reply.
> On 05/06/19 6:19 PM, Pavel Emelianov wrote:
>> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
>>> Hi,
>>>
>>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
>> Yes.
>>
>>> What is specific about transition testcases?
>> They take time to accomplish, so the default run is limited.
> So checking default is enough for patch submission?

Yes, but we still ask submitters to test their patches with as many tests as possible :)

>>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>>> - Moved out unfreeze code from cr_pre_dump_finish()
>>> - Moved out collect_mappings() from pre_dump_one_task
>>>
>>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>>
>>> Share your thoughts on this approach. I am attaching patch-file for reference.
>>> Hi,
>>>
>>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
>>> What is specific about transition testcases?
>>>
>>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>>> - Moved out unfreeze code from cr_pre_dump_finish()
>>> - Moved out collect_mappings() from pre_dump_one_task
>> Please, describe why you need this. Current code does "fetching vmas between freeze and unfreeze", 
>> what's wrong with it so you need this change?
> 
> In current approach TIME_FROZEN encapsulates TIME_MEMDUMP, like:
> 
>    *start(TIME_FROZEN)*
> 
> *        collect mappings*
> 
> *        start(TIME_MEMDUMP)*
> 
> *                step0: page map cache init*
> 
> *                step1: generate pagemap      **
> *
> 
> *                step2: grab pages into pipes*
> 
> *         end(TIME_MEMDUMP)*
> 
> *         unfreeze pstree
> *
> 
> *    end(TIME_FROZEN)*

Yes

> and the objective I perceived, is something like:
> 
>     *start(TIME_FROZEN)           ----}    ***
> 
> *            collect mappings            }==> Current patch 
> *
> 
> *            unfreeze pstree             }
> *
> 
> *    **end(TIME_FROZEN)            ----}*
> 
> *    start(TIME_MEMDUMP) *
> 
> *            step0: page map cache init*
> 
> **
> 
> *            step1: generate pagemap      **
> *
> 
> **
> 
> *            step2: grab pages into pipes*
> 
> **
> 
> *    end(TIME_MEMDUMP)*

Ah, I see. You need to move the dumping pages out of tasks-are-frozen state. This single patch
is incorrect in this case, as once tasks will start doing something with their memory the current
memory predumping code will fail and exit, while the code we're looking for would handle memory
dump errors carefully without stopping the whole action.

> *
> *
> 
> With reference to the comment "fetching vmas between freeze and unfreeze", I think this is what is needed:
> 
> *    start(TIME_FROZEN)*
> 
> *        collect mappings*
> 
> *        start(TIME_MEMDUMP)*
> 
> *                step0: page map cache init*
> 
> *                step1: generate pagemap      **
> *
> 
> *         end(TIME_MEMDUMP)*
> 
> *         unfreeze pstree
> *
> 
> *    end(TIME_FROZEN)*
> 
> 
> **    step2: grab pages into user buffer using system_vm_readv and resolve races
> **
> 
>  Please confirm if above design is right?

Yes, with the "grab pages into user buffer" part the patch SET would be correct.

>  So, I don't have to worry about unfreezing after collection? Let it happen the way it is now.

Yes, but once you unfroze the tree you must handle memory dump errors in a grace manner (by
updating the pagemap.img entries respectively), not the way it is done now (the whole criu just
exits with an error message).

> My immediate task will be getting pipes out of picture. I will start with "commit bb98a82" as reference, as
> 
> suggested by Radostin in other thread.
> 
>>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>>
>>> Share your thoughts on this approach. I am attaching patch-file for reference.
>>>
>>> -Abhishek
>>>
>>> 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
>>>
>>> >From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001
>> Please mind the https://criu.org/How_to_submit_patches article. Patch should go inline, not attached,
>> and properly formatted. Also comments are inline.
> Sure, I will take care of same when sending upstream patches. This patch was just to discuss approach and not meant in any way to be upstream.

Cool :)

>>> From: abhidubey4178 <abhishekdubey4178940@gmail.com>
>>> Date: Tue, 4 Jun 2019 19:59:21 +0530
>>> Subject: [PATCH] freezing target task to collect memory mappings, followed by
>>>  unfreeze
>>>
>>> ---
>>>  criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
>>>  criu/include/pstree.h |   1 +
>>>  2 files changed, 78 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
>>> index 7f2e5edf..40bc7594 100644
>>> --- a/criu/cr-dump.c
>>> +++ b/criu/cr-dump.c
>>> @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>>  	if (item->pid->state == TASK_DEAD)
>>>  		return 0;
>>>  
>>> -	ret = collect_mappings(pid, &vmas, NULL);
>> Up above there have been left the now unused vmas variable.
> ok.
>>> -	if (ret) {
>>> +	if (!item->vmas) {
>>>  		pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>>>  		goto err;
>>>  	}
>>>  
>>>  	ret = -1;
>>> -	parasite_ctl = parasite_infect_seized(pid, item, &vmas);
>>> +	parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
>>>  	if (!parasite_ctl) {
>>>  		pr_err("Can't infect (pid: %d) with parasite\n", pid);
>>>  		goto err_free;
>>>  	}
>>>  
>>> -	ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
>>> +	ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
>>>  	if (ret) {
>>>  		pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
>>>  		goto err_cure;
>>> @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>>  	mdc.stat = NULL;
>>>  	mdc.parent_ie = parent_ie;
>>>  
>>> -	ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
>>> +	ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
>>>  	if (ret)
>>>  		goto err_cure;
>>>  
>>>  	if (compel_cure_remote(parasite_ctl))
>>>  		pr_err("Can't cure (pid: %d) from parasite\n", pid);
>>>  err_free:
>>> -	free_mappings(&vmas);
>>> +	free_mappings(item->vmas);
>>>  err:
>>>  	return ret;
>>>  
>>> @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
>>>  
>>>  static int cr_pre_dump_finish(int status)
>>>  {
>>> -	InventoryEntry he = INVENTORY_ENTRY__INIT;
>>>  	struct pstree_item *item;
>>>  	int ret;
>>>  
>>> -	/*
>>> -	 * Restore registers for tasks only. The threads have not been
>>> -	 * infected. Therefore, the thread register sets have not been changed.
>>> -	 */
>>> -	ret = arch_set_thread_regs(root_item, false);
>>> -	if (ret)
>>> -		goto err;
>>> -
>>> -	ret = inventory_save_uptime(&he);
>>> -	if (ret)
>>> -		goto err;
>>> -
>>> -	pstree_switch_state(root_item, TASK_ALIVE);
>>> -
>>> -	timing_stop(TIME_FROZEN);
>>> -
>>>  	if (status < 0) {
>>>  		ret = status;
>>>  		goto err;
>>> @@ -1540,9 +1522,6 @@ err:
>>>  	if (bfd_flush_images())
>>>  		ret = -1;
>>>  
>>> -	if (write_img_inventory(&he))
>>> -		ret = -1;
>>> -
>> You write inventory too early, I'm not 100% sure this is correct.
>>
>>>  	if (ret)
>>>  		pr_err("Pre-dumping FAILED.\n");
>>>  	else {
>>> @@ -1552,6 +1531,68 @@ err:
>>>  	return ret;
>>>  }
>>>  
>>> +
>>> +static int pre_dump_collect_vmas(struct pstree_item *item)
>>> +{
>>> +       pid_t pid = item->pid->real;
>>> +       struct vm_area_list *vmas;
>>> +       int ret = -1;
>>> +
>>> +       vmas = xmalloc(sizeof(struct vm_area_list));
>> This memory is never free-d.
> Got it.
>>> +       if(!vmas)
>>> +            return -1;
>>> +       INIT_LIST_HEAD(&vmas->h);
>>> +       vmas->nr = 0;
>>> +
>>> +       pr_info("========================================\n");
>>> +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
>>> +       pr_info("========================================\n");
>>> +
>>> +       if (item->pid->state == TASK_STOPPED) {
>>> +               pr_warn("Stopped tasks are not supported\n");
>>> +                    return 0;
>>> +            }
>> Formatting.
>>
>>> +
>>> +       if (item->pid->state == TASK_DEAD)
>>> +                    return 0;
>> Vmas allocation can happen here.
> Got it.
>>> +
>>> +       ret = collect_mappings(pid, vmas, NULL);
>>> +       if (ret) {
>>> +               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>>> +               return -1;
>>> +            }
>>> +
>>> +       item->vmas = vmas;
>>> +       return 0;
>>> +}
>>> +
>>> +
>>> +static int unfreeze_pstree()
>>> +{
>>> +	InventoryEntry he = INVENTORY_ENTRY__INIT;
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * Restore registers for tasks only. The threads have not been
>>> +	 * infected. Therefore, the thread register sets have not been changed.
>>> +	 */
>>> +	ret = arch_set_thread_regs(root_item, false);
>>> +	if (ret)
>>> +		return -1;
>>> +
>>> +	ret = inventory_save_uptime(&he);
>>> +	if (ret)
>>> +		return -1;
>>> +
>>> +	if (write_img_inventory(&he))
>>> +		return -1;
>>> +
>>> +	pstree_switch_state(root_item, TASK_ALIVE);
>>> +	timing_stop(TIME_FROZEN);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  int cr_pre_dump_tasks(pid_t pid)
>>>  {
>>>  	InventoryEntry *parent_ie = NULL;
>>> @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
>>>  	/* Errors handled later in detect_pid_reuse */
>>>  	parent_ie = get_parent_inventory();
>>>  
>>> +	/* Collecting VMAs of all processes in pstree */
>>> +	for_each_pstree_item(item)
>>> +		if (pre_dump_collect_vmas(item))
>>> +			goto err;
>>> +
>>> +	/* Unfreeze pstree */
>> This is pointless comment as it 100% repeats the subsequent function name :)
> Won't repeat!
>>> +	ret = unfreeze_pstree();
>>> +	if(ret)
>>> +		goto err;
>>> +
>>>  	for_each_pstree_item(item)
>>>  		if (pre_dump_one_task(item, parent_ie))
>>>  			goto err;
>>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
>>> index 7303c1fe..69562608 100644
>>> --- a/criu/include/pstree.h
>>> +++ b/criu/include/pstree.h
>>> @@ -30,6 +30,7 @@ struct pstree_item {
>>>  		futex_t		task_st;
>>>  		unsigned long	task_st_le_bits;
>>>  	};
>>> +	struct vm_area_list *vmas;
>> This is dump-only field, it should be placed on dmp_info instead.
> Sure. 
>>>  };
>>>  
>>>  static inline pid_t vpid(const struct pstree_item *i)
>>> -- 
>>> 2.17.1
>>>
>> -- Pavel

-- Pavel
Andrei Vagin June 10, 2019, 7:29 a.m.
On Wed, Jun 05, 2019 at 12:49:03PM +0000, Pavel Emelianov wrote:
> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
> > Hi,
> > 
> > On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> 
> Yes.

I don't understand what you mean here.

https://ci.openvz.org/job/CRIU/job/CRIU-x86_64/job/criu-dev/5439/console

Here we can see that all trasition tests were checked...

> 
> > What is specific about transition testcases?
> 
> They take time to accomplish, so the default run is limited.
> 
> > For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
> > - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
> > - Moved out unfreeze code from cr_pre_dump_finish()
> > - Moved out collect_mappings() from pre_dump_one_task
> > 
> > I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
> > 
> > Share your thoughts on this approach. I am attaching patch-file for reference.
> 
> > 
> > Hi,
> > 
> > On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> > What is specific about transition testcases?
> > 
> > For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
> > - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
> > - Moved out unfreeze code from cr_pre_dump_finish()
> > - Moved out collect_mappings() from pre_dump_one_task
> 
> Please, describe why you need this. Current code does "fetching vmas between freeze and unfreeze", 
> what's wrong with it so you need this change?
> 
> > I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
> > 
> > Share your thoughts on this approach. I am attaching patch-file for reference.
> > 
> > -Abhishek
> > 
> > 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
> > 
> > From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001
> 
> Please mind the https://criu.org/How_to_submit_patches article. Patch should go inline, not attached,
> and properly formatted. Also comments are inline.
> 
> > From: abhidubey4178 <abhishekdubey4178940@gmail.com>
> > Date: Tue, 4 Jun 2019 19:59:21 +0530
> > Subject: [PATCH] freezing target task to collect memory mappings, followed by
> >  unfreeze
> > 
> > ---
> >  criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
> >  criu/include/pstree.h |   1 +
> >  2 files changed, 78 insertions(+), 26 deletions(-)
> > 
> > diff --git a/criu/cr-dump.c b/criu/cr-dump.c
> > index 7f2e5edf..40bc7594 100644
> > --- a/criu/cr-dump.c
> > +++ b/criu/cr-dump.c
> > @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
> >  	if (item->pid->state == TASK_DEAD)
> >  		return 0;
> >  
> > -	ret = collect_mappings(pid, &vmas, NULL);
> 
> Up above there have been left the now unused vmas variable.
> 
> > -	if (ret) {
> > +	if (!item->vmas) {
> >  		pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
> >  		goto err;
> >  	}
> >  
> >  	ret = -1;
> > -	parasite_ctl = parasite_infect_seized(pid, item, &vmas);
> > +	parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
> >  	if (!parasite_ctl) {
> >  		pr_err("Can't infect (pid: %d) with parasite\n", pid);
> >  		goto err_free;
> >  	}
> >  
> > -	ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
> > +	ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
> >  	if (ret) {
> >  		pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
> >  		goto err_cure;
> > @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
> >  	mdc.stat = NULL;
> >  	mdc.parent_ie = parent_ie;
> >  
> > -	ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
> > +	ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
> >  	if (ret)
> >  		goto err_cure;
> >  
> >  	if (compel_cure_remote(parasite_ctl))
> >  		pr_err("Can't cure (pid: %d) from parasite\n", pid);
> >  err_free:
> > -	free_mappings(&vmas);
> > +	free_mappings(item->vmas);
> >  err:
> >  	return ret;
> >  
> > @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
> >  
> >  static int cr_pre_dump_finish(int status)
> >  {
> > -	InventoryEntry he = INVENTORY_ENTRY__INIT;
> >  	struct pstree_item *item;
> >  	int ret;
> >  
> > -	/*
> > -	 * Restore registers for tasks only. The threads have not been
> > -	 * infected. Therefore, the thread register sets have not been changed.
> > -	 */
> > -	ret = arch_set_thread_regs(root_item, false);
> > -	if (ret)
> > -		goto err;
> > -
> > -	ret = inventory_save_uptime(&he);
> > -	if (ret)
> > -		goto err;
> > -
> > -	pstree_switch_state(root_item, TASK_ALIVE);
> > -
> > -	timing_stop(TIME_FROZEN);
> > -
> >  	if (status < 0) {
> >  		ret = status;
> >  		goto err;
> > @@ -1540,9 +1522,6 @@ err:
> >  	if (bfd_flush_images())
> >  		ret = -1;
> >  
> > -	if (write_img_inventory(&he))
> > -		ret = -1;
> > -
> 
> You write inventory too early, I'm not 100% sure this is correct.
> 
> >  	if (ret)
> >  		pr_err("Pre-dumping FAILED.\n");
> >  	else {
> > @@ -1552,6 +1531,68 @@ err:
> >  	return ret;
> >  }
> >  
> > +
> > +static int pre_dump_collect_vmas(struct pstree_item *item)
> > +{
> > +       pid_t pid = item->pid->real;
> > +       struct vm_area_list *vmas;
> > +       int ret = -1;
> > +
> > +       vmas = xmalloc(sizeof(struct vm_area_list));
> 
> This memory is never free-d.
> 
> > +       if(!vmas)
> > +            return -1;
> > +       INIT_LIST_HEAD(&vmas->h);
> > +       vmas->nr = 0;
> > +
> > +       pr_info("========================================\n");
> > +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
> > +       pr_info("========================================\n");
> > +
> > +       if (item->pid->state == TASK_STOPPED) {
> > +               pr_warn("Stopped tasks are not supported\n");
> > +                    return 0;
> > +            }
> 
> Formatting.
> 
> > +
> > +       if (item->pid->state == TASK_DEAD)
> > +                    return 0;
> 
> Vmas allocation can happen here.
> 
> > +
> > +       ret = collect_mappings(pid, vmas, NULL);
> > +       if (ret) {
> > +               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
> > +               return -1;
> > +            }
> > +
> > +       item->vmas = vmas;
> > +       return 0;
> > +}
> > +
> > +
> > +static int unfreeze_pstree()
> > +{
> > +	InventoryEntry he = INVENTORY_ENTRY__INIT;
> > +	int ret;
> > +
> > +	/*
> > +	 * Restore registers for tasks only. The threads have not been
> > +	 * infected. Therefore, the thread register sets have not been changed.
> > +	 */
> > +	ret = arch_set_thread_regs(root_item, false);
> > +	if (ret)
> > +		return -1;
> > +
> > +	ret = inventory_save_uptime(&he);
> > +	if (ret)
> > +		return -1;
> > +
> > +	if (write_img_inventory(&he))
> > +		return -1;
> > +
> > +	pstree_switch_state(root_item, TASK_ALIVE);
> > +	timing_stop(TIME_FROZEN);
> > +
> > +	return 0;
> > +}
> > +
> >  int cr_pre_dump_tasks(pid_t pid)
> >  {
> >  	InventoryEntry *parent_ie = NULL;
> > @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
> >  	/* Errors handled later in detect_pid_reuse */
> >  	parent_ie = get_parent_inventory();
> >  
> > +	/* Collecting VMAs of all processes in pstree */
> > +	for_each_pstree_item(item)
> > +		if (pre_dump_collect_vmas(item))
> > +			goto err;
> > +
> > +	/* Unfreeze pstree */
> 
> This is pointless comment as it 100% repeats the subsequent function name :)
> 
> > +	ret = unfreeze_pstree();
> > +	if(ret)
> > +		goto err;
> > +
> >  	for_each_pstree_item(item)
> >  		if (pre_dump_one_task(item, parent_ie))
> >  			goto err;
> > diff --git a/criu/include/pstree.h b/criu/include/pstree.h
> > index 7303c1fe..69562608 100644
> > --- a/criu/include/pstree.h
> > +++ b/criu/include/pstree.h
> > @@ -30,6 +30,7 @@ struct pstree_item {
> >  		futex_t		task_st;
> >  		unsigned long	task_st_le_bits;
> >  	};
> > +	struct vm_area_list *vmas;
> 
> This is dump-only field, it should be placed on dmp_info instead.
> 
> >  };
> >  
> >  static inline pid_t vpid(const struct pstree_item *i)
> > -- 
> > 2.17.1
> > 
> 
> -- Pavel
Andrei Vagin June 10, 2019, 7:38 a.m.
On Fri, Jun 07, 2019 at 03:10:44PM +0000, Pavel Emelianov wrote:
> On 6/6/19 10:31 AM, abhishek dubey wrote:
> > Please find Inline reply.
> > On 05/06/19 6:19 PM, Pavel Emelianov wrote:
> >> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
> >>> Hi,
> >>>
> >>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> >> Yes.
> >>
> >>> What is specific about transition testcases?
> >> They take time to accomplish, so the default run is limited.
> > So checking default is enough for patch submission?
> 
> Yes, but we still ask submitters to test their patches with as many tests as possible :)

No, it isn't. A submitter should enable travis (https://travis-ci.org/)
for their github criu repo and then check that a travis build passes for
changes what are going to be submitted.


Thanks,
AV

>
Andrei Vagin June 10, 2019, 7:44 a.m.
On Mon, Jun 10, 2019 at 12:38:01AM -0700, Andrei Vagin (C) wrote:
> On Fri, Jun 07, 2019 at 03:10:44PM +0000, Pavel Emelianov wrote:
> > On 6/6/19 10:31 AM, abhishek dubey wrote:
> > > Please find Inline reply.
> > > On 05/06/19 6:19 PM, Pavel Emelianov wrote:
> > >> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
> > >>> Hi,
> > >>>
> > >>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> > >> Yes.
> > >>
> > >>> What is specific about transition testcases?
> > >> They take time to accomplish, so the default run is limited.
> > > So checking default is enough for patch submission?
> > 
> > Yes, but we still ask submitters to test their patches with as many tests as possible :)
> 
> No, it isn't. A submitter should enable travis (https://travis-ci.org/)
> for their github criu repo and then check that a travis build passes for
> changes what are going to be submitted.

BTW: for this patch, a travis build completed without any fails:

https://travis-ci.org/criupatchwork/criu/builds/541339078?utm_source=github_status&utm_medium=notification

Travis is executed automatically for each submitted patch series in the
mailing list.

> 
> 
> Thanks,
> AV
> 
> >
Abhishek Dubey June 10, 2019, 9:51 a.m.
On 10/06/19 12:59 PM, Andrei Vagin (C) wrote:
> On Wed, Jun 05, 2019 at 12:49:03PM +0000, Pavel Emelianov wrote:
>> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
>>> Hi,
>>>
>>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
>> Yes.
> I don't understand what you mean here.
>
> https://ci.openvz.org/job/CRIU/job/CRIU-x86_64/job/criu-dev/5439/console
>
> Here we can see that all trasition tests were checked...
Here I refer to locally running zdtm tests as given on 
https://www.criu.org/ZDTM_test_suite
>
>>> What is specific about transition testcases?
>> They take time to accomplish, so the default run is limited.
>>
>>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>>> - Moved out unfreeze code from cr_pre_dump_finish()
>>> - Moved out collect_mappings() from pre_dump_one_task
>>>
>>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>>
>>> Share your thoughts on this approach. I am attaching patch-file for reference.
>>> Hi,
>>>
>>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
>>> What is specific about transition testcases?
>>>
>>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>>> - Moved out unfreeze code from cr_pre_dump_finish()
>>> - Moved out collect_mappings() from pre_dump_one_task
>> Please, describe why you need this. Current code does "fetching vmas between freeze and unfreeze",
>> what's wrong with it so you need this change?
>>
>>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>>
>>> Share your thoughts on this approach. I am attaching patch-file for reference.
>>>
>>> -Abhishek
>>>
>>> 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
>>>
>>>  From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001
>> Please mind the https://criu.org/How_to_submit_patches article. Patch should go inline, not attached,
>> and properly formatted. Also comments are inline.
>>
>>> From: abhidubey4178 <abhishekdubey4178940@gmail.com>
>>> Date: Tue, 4 Jun 2019 19:59:21 +0530
>>> Subject: [PATCH] freezing target task to collect memory mappings, followed by
>>>   unfreeze
>>>
>>> ---
>>>   criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
>>>   criu/include/pstree.h |   1 +
>>>   2 files changed, 78 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
>>> index 7f2e5edf..40bc7594 100644
>>> --- a/criu/cr-dump.c
>>> +++ b/criu/cr-dump.c
>>> @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>>   	if (item->pid->state == TASK_DEAD)
>>>   		return 0;
>>>   
>>> -	ret = collect_mappings(pid, &vmas, NULL);
>> Up above there have been left the now unused vmas variable.
>>
>>> -	if (ret) {
>>> +	if (!item->vmas) {
>>>   		pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>>>   		goto err;
>>>   	}
>>>   
>>>   	ret = -1;
>>> -	parasite_ctl = parasite_infect_seized(pid, item, &vmas);
>>> +	parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
>>>   	if (!parasite_ctl) {
>>>   		pr_err("Can't infect (pid: %d) with parasite\n", pid);
>>>   		goto err_free;
>>>   	}
>>>   
>>> -	ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
>>> +	ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
>>>   	if (ret) {
>>>   		pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
>>>   		goto err_cure;
>>> @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>>   	mdc.stat = NULL;
>>>   	mdc.parent_ie = parent_ie;
>>>   
>>> -	ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
>>> +	ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
>>>   	if (ret)
>>>   		goto err_cure;
>>>   
>>>   	if (compel_cure_remote(parasite_ctl))
>>>   		pr_err("Can't cure (pid: %d) from parasite\n", pid);
>>>   err_free:
>>> -	free_mappings(&vmas);
>>> +	free_mappings(item->vmas);
>>>   err:
>>>   	return ret;
>>>   
>>> @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
>>>   
>>>   static int cr_pre_dump_finish(int status)
>>>   {
>>> -	InventoryEntry he = INVENTORY_ENTRY__INIT;
>>>   	struct pstree_item *item;
>>>   	int ret;
>>>   
>>> -	/*
>>> -	 * Restore registers for tasks only. The threads have not been
>>> -	 * infected. Therefore, the thread register sets have not been changed.
>>> -	 */
>>> -	ret = arch_set_thread_regs(root_item, false);
>>> -	if (ret)
>>> -		goto err;
>>> -
>>> -	ret = inventory_save_uptime(&he);
>>> -	if (ret)
>>> -		goto err;
>>> -
>>> -	pstree_switch_state(root_item, TASK_ALIVE);
>>> -
>>> -	timing_stop(TIME_FROZEN);
>>> -
>>>   	if (status < 0) {
>>>   		ret = status;
>>>   		goto err;
>>> @@ -1540,9 +1522,6 @@ err:
>>>   	if (bfd_flush_images())
>>>   		ret = -1;
>>>   
>>> -	if (write_img_inventory(&he))
>>> -		ret = -1;
>>> -
>> You write inventory too early, I'm not 100% sure this is correct.
>>
>>>   	if (ret)
>>>   		pr_err("Pre-dumping FAILED.\n");
>>>   	else {
>>> @@ -1552,6 +1531,68 @@ err:
>>>   	return ret;
>>>   }
>>>   
>>> +
>>> +static int pre_dump_collect_vmas(struct pstree_item *item)
>>> +{
>>> +       pid_t pid = item->pid->real;
>>> +       struct vm_area_list *vmas;
>>> +       int ret = -1;
>>> +
>>> +       vmas = xmalloc(sizeof(struct vm_area_list));
>> This memory is never free-d.
>>
>>> +       if(!vmas)
>>> +            return -1;
>>> +       INIT_LIST_HEAD(&vmas->h);
>>> +       vmas->nr = 0;
>>> +
>>> +       pr_info("========================================\n");
>>> +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
>>> +       pr_info("========================================\n");
>>> +
>>> +       if (item->pid->state == TASK_STOPPED) {
>>> +               pr_warn("Stopped tasks are not supported\n");
>>> +                    return 0;
>>> +            }
>> Formatting.
>>
>>> +
>>> +       if (item->pid->state == TASK_DEAD)
>>> +                    return 0;
>> Vmas allocation can happen here.
>>
>>> +
>>> +       ret = collect_mappings(pid, vmas, NULL);
>>> +       if (ret) {
>>> +               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>>> +               return -1;
>>> +            }
>>> +
>>> +       item->vmas = vmas;
>>> +       return 0;
>>> +}
>>> +
>>> +
>>> +static int unfreeze_pstree()
>>> +{
>>> +	InventoryEntry he = INVENTORY_ENTRY__INIT;
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * Restore registers for tasks only. The threads have not been
>>> +	 * infected. Therefore, the thread register sets have not been changed.
>>> +	 */
>>> +	ret = arch_set_thread_regs(root_item, false);
>>> +	if (ret)
>>> +		return -1;
>>> +
>>> +	ret = inventory_save_uptime(&he);
>>> +	if (ret)
>>> +		return -1;
>>> +
>>> +	if (write_img_inventory(&he))
>>> +		return -1;
>>> +
>>> +	pstree_switch_state(root_item, TASK_ALIVE);
>>> +	timing_stop(TIME_FROZEN);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   int cr_pre_dump_tasks(pid_t pid)
>>>   {
>>>   	InventoryEntry *parent_ie = NULL;
>>> @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
>>>   	/* Errors handled later in detect_pid_reuse */
>>>   	parent_ie = get_parent_inventory();
>>>   
>>> +	/* Collecting VMAs of all processes in pstree */
>>> +	for_each_pstree_item(item)
>>> +		if (pre_dump_collect_vmas(item))
>>> +			goto err;
>>> +
>>> +	/* Unfreeze pstree */
>> This is pointless comment as it 100% repeats the subsequent function name :)
>>
>>> +	ret = unfreeze_pstree();
>>> +	if(ret)
>>> +		goto err;
>>> +
>>>   	for_each_pstree_item(item)
>>>   		if (pre_dump_one_task(item, parent_ie))
>>>   			goto err;
>>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
>>> index 7303c1fe..69562608 100644
>>> --- a/criu/include/pstree.h
>>> +++ b/criu/include/pstree.h
>>> @@ -30,6 +30,7 @@ struct pstree_item {
>>>   		futex_t		task_st;
>>>   		unsigned long	task_st_le_bits;
>>>   	};
>>> +	struct vm_area_list *vmas;
>> This is dump-only field, it should be placed on dmp_info instead.
>>
>>>   };
>>>   
>>>   static inline pid_t vpid(const struct pstree_item *i)
>>> -- 
>>> 2.17.1
>>>
>> -- Pavel
Andrei Vagin June 10, 2019, 5:20 p.m.
On Mon, Jun 10, 2019 at 2:50 AM abhishek dubey
<dubeyabhishek777@gmail.com> wrote:
>
>
> On 10/06/19 12:59 PM, Andrei Vagin (C) wrote:
> > On Wed, Jun 05, 2019 at 12:49:03PM +0000, Pavel Emelianov wrote:
> >> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
> >>> Hi,
> >>>
> >>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> >> Yes.
> > I don't understand what you mean here.
> >
> > https://ci.openvz.org/job/CRIU/job/CRIU-x86_64/job/criu-dev/5439/console
> >
> > Here we can see that all trasition tests were checked...
> Here I refer to locally running zdtm tests as given on
> https://www.criu.org/ZDTM_test_suite

Could you show output for zdtm.py run -a?

> >
> >>> What is specific about transition testcases?
> >> They take time to accomplish, so the default run is limited.
> >>
> >>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
> >>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
> >>> - Moved out unfreeze code from cr_pre_dump_finish()
> >>> - Moved out collect_mappings() from pre_dump_one_task
> >>>
> >>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
> >>>
> >>> Share your thoughts on this approach. I am attaching patch-file for reference.
> >>> Hi,
> >>>
> >>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
> >>> What is specific about transition testcases?
> >>>
> >>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
> >>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
> >>> - Moved out unfreeze code from cr_pre_dump_finish()
> >>> - Moved out collect_mappings() from pre_dump_one_task
> >> Please, describe why you need this. Current code does "fetching vmas between freeze and unfreeze",
> >> what's wrong with it so you need this change?
> >>
> >>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
> >>>
> >>> Share your thoughts on this approach. I am attaching patch-file for reference.
> >>>
> >>> -Abhishek
> >>>
> >>> 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
> >>>
> >>>  From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001
> >> Please mind the https://criu.org/How_to_submit_patches article. Patch should go inline, not attached,
> >> and properly formatted. Also comments are inline.
> >>
> >>> From: abhidubey4178 <abhishekdubey4178940@gmail.com>
> >>> Date: Tue, 4 Jun 2019 19:59:21 +0530
> >>> Subject: [PATCH] freezing target task to collect memory mappings, followed by
> >>>   unfreeze
> >>>
> >>> ---
> >>>   criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
> >>>   criu/include/pstree.h |   1 +
> >>>   2 files changed, 78 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
> >>> index 7f2e5edf..40bc7594 100644
> >>> --- a/criu/cr-dump.c
> >>> +++ b/criu/cr-dump.c
> >>> @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
> >>>     if (item->pid->state == TASK_DEAD)
> >>>             return 0;
> >>>
> >>> -   ret = collect_mappings(pid, &vmas, NULL);
> >> Up above there have been left the now unused vmas variable.
> >>
> >>> -   if (ret) {
> >>> +   if (!item->vmas) {
> >>>             pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
> >>>             goto err;
> >>>     }
> >>>
> >>>     ret = -1;
> >>> -   parasite_ctl = parasite_infect_seized(pid, item, &vmas);
> >>> +   parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
> >>>     if (!parasite_ctl) {
> >>>             pr_err("Can't infect (pid: %d) with parasite\n", pid);
> >>>             goto err_free;
> >>>     }
> >>>
> >>> -   ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
> >>> +   ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
> >>>     if (ret) {
> >>>             pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
> >>>             goto err_cure;
> >>> @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
> >>>     mdc.stat = NULL;
> >>>     mdc.parent_ie = parent_ie;
> >>>
> >>> -   ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
> >>> +   ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
> >>>     if (ret)
> >>>             goto err_cure;
> >>>
> >>>     if (compel_cure_remote(parasite_ctl))
> >>>             pr_err("Can't cure (pid: %d) from parasite\n", pid);
> >>>   err_free:
> >>> -   free_mappings(&vmas);
> >>> +   free_mappings(item->vmas);
> >>>   err:
> >>>     return ret;
> >>>
> >>> @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
> >>>
> >>>   static int cr_pre_dump_finish(int status)
> >>>   {
> >>> -   InventoryEntry he = INVENTORY_ENTRY__INIT;
> >>>     struct pstree_item *item;
> >>>     int ret;
> >>>
> >>> -   /*
> >>> -    * Restore registers for tasks only. The threads have not been
> >>> -    * infected. Therefore, the thread register sets have not been changed.
> >>> -    */
> >>> -   ret = arch_set_thread_regs(root_item, false);
> >>> -   if (ret)
> >>> -           goto err;
> >>> -
> >>> -   ret = inventory_save_uptime(&he);
> >>> -   if (ret)
> >>> -           goto err;
> >>> -
> >>> -   pstree_switch_state(root_item, TASK_ALIVE);
> >>> -
> >>> -   timing_stop(TIME_FROZEN);
> >>> -
> >>>     if (status < 0) {
> >>>             ret = status;
> >>>             goto err;
> >>> @@ -1540,9 +1522,6 @@ err:
> >>>     if (bfd_flush_images())
> >>>             ret = -1;
> >>>
> >>> -   if (write_img_inventory(&he))
> >>> -           ret = -1;
> >>> -
> >> You write inventory too early, I'm not 100% sure this is correct.
> >>
> >>>     if (ret)
> >>>             pr_err("Pre-dumping FAILED.\n");
> >>>     else {
> >>> @@ -1552,6 +1531,68 @@ err:
> >>>     return ret;
> >>>   }
> >>>
> >>> +
> >>> +static int pre_dump_collect_vmas(struct pstree_item *item)
> >>> +{
> >>> +       pid_t pid = item->pid->real;
> >>> +       struct vm_area_list *vmas;
> >>> +       int ret = -1;
> >>> +
> >>> +       vmas = xmalloc(sizeof(struct vm_area_list));
> >> This memory is never free-d.
> >>
> >>> +       if(!vmas)
> >>> +            return -1;
> >>> +       INIT_LIST_HEAD(&vmas->h);
> >>> +       vmas->nr = 0;
> >>> +
> >>> +       pr_info("========================================\n");
> >>> +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
> >>> +       pr_info("========================================\n");
> >>> +
> >>> +       if (item->pid->state == TASK_STOPPED) {
> >>> +               pr_warn("Stopped tasks are not supported\n");
> >>> +                    return 0;
> >>> +            }
> >> Formatting.
> >>
> >>> +
> >>> +       if (item->pid->state == TASK_DEAD)
> >>> +                    return 0;
> >> Vmas allocation can happen here.
> >>
> >>> +
> >>> +       ret = collect_mappings(pid, vmas, NULL);
> >>> +       if (ret) {
> >>> +               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
> >>> +               return -1;
> >>> +            }
> >>> +
> >>> +       item->vmas = vmas;
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +
> >>> +static int unfreeze_pstree()
> >>> +{
> >>> +   InventoryEntry he = INVENTORY_ENTRY__INIT;
> >>> +   int ret;
> >>> +
> >>> +   /*
> >>> +    * Restore registers for tasks only. The threads have not been
> >>> +    * infected. Therefore, the thread register sets have not been changed.
> >>> +    */
> >>> +   ret = arch_set_thread_regs(root_item, false);
> >>> +   if (ret)
> >>> +           return -1;
> >>> +
> >>> +   ret = inventory_save_uptime(&he);
> >>> +   if (ret)
> >>> +           return -1;
> >>> +
> >>> +   if (write_img_inventory(&he))
> >>> +           return -1;
> >>> +
> >>> +   pstree_switch_state(root_item, TASK_ALIVE);
> >>> +   timing_stop(TIME_FROZEN);
> >>> +
> >>> +   return 0;
> >>> +}
> >>> +
> >>>   int cr_pre_dump_tasks(pid_t pid)
> >>>   {
> >>>     InventoryEntry *parent_ie = NULL;
> >>> @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
> >>>     /* Errors handled later in detect_pid_reuse */
> >>>     parent_ie = get_parent_inventory();
> >>>
> >>> +   /* Collecting VMAs of all processes in pstree */
> >>> +   for_each_pstree_item(item)
> >>> +           if (pre_dump_collect_vmas(item))
> >>> +                   goto err;
> >>> +
> >>> +   /* Unfreeze pstree */
> >> This is pointless comment as it 100% repeats the subsequent function name :)
> >>
> >>> +   ret = unfreeze_pstree();
> >>> +   if(ret)
> >>> +           goto err;
> >>> +
> >>>     for_each_pstree_item(item)
> >>>             if (pre_dump_one_task(item, parent_ie))
> >>>                     goto err;
> >>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
> >>> index 7303c1fe..69562608 100644
> >>> --- a/criu/include/pstree.h
> >>> +++ b/criu/include/pstree.h
> >>> @@ -30,6 +30,7 @@ struct pstree_item {
> >>>             futex_t         task_st;
> >>>             unsigned long   task_st_le_bits;
> >>>     };
> >>> +   struct vm_area_list *vmas;
> >> This is dump-only field, it should be placed on dmp_info instead.
> >>
> >>>   };
> >>>
> >>>   static inline pid_t vpid(const struct pstree_item *i)
> >>> --
> >>> 2.17.1
> >>>
> >> -- Pavel
Abhishek Dubey June 11, 2019, 6:22 p.m.
Summary of test/zdtm.py run -a --ignore-taint --keep-going is
################## ALL TEST(S) PASSED (TOTAL 204/SKIPPED 21)
###################

Sending complete console output separately.

On Mon, Jun 10, 2019 at 10:50 PM Andrei Vagin <avagin@gmail.com> wrote:

> On Mon, Jun 10, 2019 at 2:50 AM abhishek dubey
> <dubeyabhishek777@gmail.com> wrote:
> >
> >
> > On 10/06/19 12:59 PM, Andrei Vagin (C) wrote:
> > > On Wed, Jun 05, 2019 at 12:49:03PM +0000, Pavel Emelianov wrote:
> > >> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
> > >>> Hi,
> > >>>
> > >>> On running "zdtm.py run -a", testcases of zdtm/static directory are
> checked and that of zdtm/transition are skipped. Do all testcases of
> transition directory need manual run?
> > >> Yes.
> > > I don't understand what you mean here.
> > >
> > >
> https://ci.openvz.org/job/CRIU/job/CRIU-x86_64/job/criu-dev/5439/console
> > >
> > > Here we can see that all trasition tests were checked...
> > Here I refer to locally running zdtm tests as given on
> > https://www.criu.org/ZDTM_test_suite
>
> Could you show output for zdtm.py run -a?
>
> > >
> > >>> What is specific about transition testcases?
> > >> They take time to accomplish, so the default run is limited.
> > >>
> > >>> For the first step of fetching VMAs between freeze and unfreeze of
> pre-dump:
> > >>> - To maintain correspondence between PID and collected VMA, added
> new entry in struct pstree_item
> > >>> - Moved out unfreeze code from cr_pre_dump_finish()
> > >>> - Moved out collect_mappings() from pre_dump_one_task
> > >>>
> > >>> I have tested these changes with env00-basic example and zdtm.py
> test-suite. All test-cases under zdtm/static directory are Passing.
> Remaining transition test cases are skipped.
> > >>>
> > >>> Share your thoughts on this approach. I am attaching patch-file for
> reference.
> > >>> Hi,
> > >>>
> > >>> On running "zdtm.py run -a", testcases of zdtm/static directory are
> checked and that of zdtm/transition are skipped. Do all testcases of
> transition directory need manual run?
> > >>> What is specific about transition testcases?
> > >>>
> > >>> For the first step of fetching VMAs between freeze and unfreeze of
> pre-dump:
> > >>> - To maintain correspondence between PID and collected VMA, added
> new entry in struct pstree_item
> > >>> - Moved out unfreeze code from cr_pre_dump_finish()
> > >>> - Moved out collect_mappings() from pre_dump_one_task
> > >> Please, describe why you need this. Current code does "fetching vmas
> between freeze and unfreeze",
> > >> what's wrong with it so you need this change?
> > >>
> > >>> I have tested these changes with env00-basic example and zdtm.py
> test-suite. All test-cases under zdtm/static directory are Passing.
> Remaining transition test cases are skipped.
> > >>>
> > >>> Share your thoughts on this approach. I am attaching patch-file for
> reference.
> > >>>
> > >>> -Abhishek
> > >>>
> > >>> 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
> > >>>
> > >>>  From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00
> 2001
> > >> Please mind the https://criu.org/How_to_submit_patches article.
> Patch should go inline, not attached,
> > >> and properly formatted. Also comments are inline.
> > >>
> > >>> From: abhidubey4178 <abhishekdubey4178940@gmail.com>
> > >>> Date: Tue, 4 Jun 2019 19:59:21 +0530
> > >>> Subject: [PATCH] freezing target task to collect memory mappings,
> followed by
> > >>>   unfreeze
> > >>>
> > >>> ---
> > >>>   criu/cr-dump.c        | 103
> +++++++++++++++++++++++++++++++-----------
> > >>>   criu/include/pstree.h |   1 +
> > >>>   2 files changed, 78 insertions(+), 26 deletions(-)
> > >>>
> > >>> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
> > >>> index 7f2e5edf..40bc7594 100644
> > >>> --- a/criu/cr-dump.c
> > >>> +++ b/criu/cr-dump.c
> > >>> @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct
> pstree_item *item, InventoryEntry *parent_ie
> > >>>     if (item->pid->state == TASK_DEAD)
> > >>>             return 0;
> > >>>
> > >>> -   ret = collect_mappings(pid, &vmas, NULL);
> > >> Up above there have been left the now unused vmas variable.
> > >>
> > >>> -   if (ret) {
> > >>> +   if (!item->vmas) {
> > >>>             pr_err("Collect mappings (pid: %d) failed with %d\n",
> pid, ret);
> > >>>             goto err;
> > >>>     }
> > >>>
> > >>>     ret = -1;
> > >>> -   parasite_ctl = parasite_infect_seized(pid, item, &vmas);
> > >>> +   parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
> > >>>     if (!parasite_ctl) {
> > >>>             pr_err("Can't infect (pid: %d) with parasite\n", pid);
> > >>>             goto err_free;
> > >>>     }
> > >>>
> > >>> -   ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
> > >>> +   ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
> > >>>     if (ret) {
> > >>>             pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
> > >>>             goto err_cure;
> > >>> @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct
> pstree_item *item, InventoryEntry *parent_ie
> > >>>     mdc.stat = NULL;
> > >>>     mdc.parent_ie = parent_ie;
> > >>>
> > >>> -   ret = parasite_dump_pages_seized(item, &vmas, &mdc,
> parasite_ctl);
> > >>> +   ret = parasite_dump_pages_seized(item, item->vmas, &mdc,
> parasite_ctl);
> > >>>     if (ret)
> > >>>             goto err_cure;
> > >>>
> > >>>     if (compel_cure_remote(parasite_ctl))
> > >>>             pr_err("Can't cure (pid: %d) from parasite\n", pid);
> > >>>   err_free:
> > >>> -   free_mappings(&vmas);
> > >>> +   free_mappings(item->vmas);
> > >>>   err:
> > >>>     return ret;
> > >>>
> > >>> @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
> > >>>
> > >>>   static int cr_pre_dump_finish(int status)
> > >>>   {
> > >>> -   InventoryEntry he = INVENTORY_ENTRY__INIT;
> > >>>     struct pstree_item *item;
> > >>>     int ret;
> > >>>
> > >>> -   /*
> > >>> -    * Restore registers for tasks only. The threads have not been
> > >>> -    * infected. Therefore, the thread register sets have not been
> changed.
> > >>> -    */
> > >>> -   ret = arch_set_thread_regs(root_item, false);
> > >>> -   if (ret)
> > >>> -           goto err;
> > >>> -
> > >>> -   ret = inventory_save_uptime(&he);
> > >>> -   if (ret)
> > >>> -           goto err;
> > >>> -
> > >>> -   pstree_switch_state(root_item, TASK_ALIVE);
> > >>> -
> > >>> -   timing_stop(TIME_FROZEN);
> > >>> -
> > >>>     if (status < 0) {
> > >>>             ret = status;
> > >>>             goto err;
> > >>> @@ -1540,9 +1522,6 @@ err:
> > >>>     if (bfd_flush_images())
> > >>>             ret = -1;
> > >>>
> > >>> -   if (write_img_inventory(&he))
> > >>> -           ret = -1;
> > >>> -
> > >> You write inventory too early, I'm not 100% sure this is correct.
> > >>
> > >>>     if (ret)
> > >>>             pr_err("Pre-dumping FAILED.\n");
> > >>>     else {
> > >>> @@ -1552,6 +1531,68 @@ err:
> > >>>     return ret;
> > >>>   }
> > >>>
> > >>> +
> > >>> +static int pre_dump_collect_vmas(struct pstree_item *item)
> > >>> +{
> > >>> +       pid_t pid = item->pid->real;
> > >>> +       struct vm_area_list *vmas;
> > >>> +       int ret = -1;
> > >>> +
> > >>> +       vmas = xmalloc(sizeof(struct vm_area_list));
> > >> This memory is never free-d.
> > >>
> > >>> +       if(!vmas)
> > >>> +            return -1;
> > >>> +       INIT_LIST_HEAD(&vmas->h);
> > >>> +       vmas->nr = 0;
> > >>> +
> > >>> +       pr_info("========================================\n");
> > >>> +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
> > >>> +       pr_info("========================================\n");
> > >>> +
> > >>> +       if (item->pid->state == TASK_STOPPED) {
> > >>> +               pr_warn("Stopped tasks are not supported\n");
> > >>> +                    return 0;
> > >>> +            }
> > >> Formatting.
> > >>
> > >>> +
> > >>> +       if (item->pid->state == TASK_DEAD)
> > >>> +                    return 0;
> > >> Vmas allocation can happen here.
> > >>
> > >>> +
> > >>> +       ret = collect_mappings(pid, vmas, NULL);
> > >>> +       if (ret) {
> > >>> +               pr_err("Collect mappings (pid: %d) failed with
> %d\n", pid, ret);
> > >>> +               return -1;
> > >>> +            }
> > >>> +
> > >>> +       item->vmas = vmas;
> > >>> +       return 0;
> > >>> +}
> > >>> +
> > >>> +
> > >>> +static int unfreeze_pstree()
> > >>> +{
> > >>> +   InventoryEntry he = INVENTORY_ENTRY__INIT;
> > >>> +   int ret;
> > >>> +
> > >>> +   /*
> > >>> +    * Restore registers for tasks only. The threads have not been
> > >>> +    * infected. Therefore, the thread register sets have not been
> changed.
> > >>> +    */
> > >>> +   ret = arch_set_thread_regs(root_item, false);
> > >>> +   if (ret)
> > >>> +           return -1;
> > >>> +
> > >>> +   ret = inventory_save_uptime(&he);
> > >>> +   if (ret)
> > >>> +           return -1;
> > >>> +
> > >>> +   if (write_img_inventory(&he))
> > >>> +           return -1;
> > >>> +
> > >>> +   pstree_switch_state(root_item, TASK_ALIVE);
> > >>> +   timing_stop(TIME_FROZEN);
> > >>> +
> > >>> +   return 0;
> > >>> +}
> > >>> +
> > >>>   int cr_pre_dump_tasks(pid_t pid)
> > >>>   {
> > >>>     InventoryEntry *parent_ie = NULL;
> > >>> @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
> > >>>     /* Errors handled later in detect_pid_reuse */
> > >>>     parent_ie = get_parent_inventory();
> > >>>
> > >>> +   /* Collecting VMAs of all processes in pstree */
> > >>> +   for_each_pstree_item(item)
> > >>> +           if (pre_dump_collect_vmas(item))
> > >>> +                   goto err;
> > >>> +
> > >>> +   /* Unfreeze pstree */
> > >> This is pointless comment as it 100% repeats the subsequent function
> name :)
> > >>
> > >>> +   ret = unfreeze_pstree();
> > >>> +   if(ret)
> > >>> +           goto err;
> > >>> +
> > >>>     for_each_pstree_item(item)
> > >>>             if (pre_dump_one_task(item, parent_ie))
> > >>>                     goto err;
> > >>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
> > >>> index 7303c1fe..69562608 100644
> > >>> --- a/criu/include/pstree.h
> > >>> +++ b/criu/include/pstree.h
> > >>> @@ -30,6 +30,7 @@ struct pstree_item {
> > >>>             futex_t         task_st;
> > >>>             unsigned long   task_st_le_bits;
> > >>>     };
> > >>> +   struct vm_area_list *vmas;
> > >> This is dump-only field, it should be placed on dmp_info instead.
> > >>
> > >>>   };
> > >>>
> > >>>   static inline pid_t vpid(const struct pstree_item *i)
> > >>> --
> > >>> 2.17.1
> > >>>
> > >> -- Pavel
>
Abhishek Dubey June 19, 2019, 10 p.m.
Hi,

In the past week, I have been working on latest code to incorporate 
Andrei's attempt for using userspace buffer.

Additional changes:

         - skipping iovs generation for READ protected areas

         - skipping parasite injection (working on it)


Debugging:

After implementing Andrei's changes and running zdtm suite, 28 test 
cases are failing. I am resolving this mismatch issue.

Error log of failed test:

=== Run 324/362 ==============-- zdtm/static/different_creds
===================== Run zdtm/static/different_creds in h 
=====================
Start test
Test is SUID
./different_creds --pidfile=different_creds.pid 
--outfile=different_creds.out
Run criu dump
=[log]=> dump/zdtm/static/different_creds/38/1/dump.log
------------------------ grep Error ------------------------
(00.007650) Fetched ack: 66 66 0
(00.007655) Transferring pages:
*(00.007658)     buf 2/1**
**(00.007669) Error (criu/page-xfer.c:498): Can't splice (4096(8192)/2)*
(00.007687) page-pipe: Killing page pipe
(00.007727) ----------------------------------------
(00.007732) Error (criu/mem.c:547): Can't dump page with parasite
(00.008371) 38 was stopped
(00.008487) Unlock network
(00.008493) Unfreezing tasks into 1
(00.008497)     Unseizing 38 into 1
(00.008510) Error (criu/cr-dump.c:1753): Dumping FAILED.
------------------------ ERROR OVER ------------------------
############## Test zdtm/static/different_creds FAIL at CRIU dump 
##############
Send the 9 signal to  38
Wait for zdtm/static/different_creds(38) to die for 0.100000

Queries:

1) Whether zdtm test suite cover test cases to check functionality of 
pre-dump? Is there a particular test case which

      needs to be passed for pre-dump functionality verification?

2) Currently I am following steps below to check if pre-dump works fine 
(since some test cases which fails in zdtm suite passes when run this way):

         - make <testcase_name>.pid

         - predump <testcase>

         - dump <testcase> pointing to predump images

         - restore <testcase>

         - make <testcase_name>.out

         look for PASS message in last step.

-Abhishek

On 12/06/19 12:19 AM, Andrei Vagin wrote:
> I think transition tests have not been compiled. zdtm.py searches all
> binaries and run them.
>
> Could you try to run make -C test/zdtm and then try to run zdtm.py run -a again.
>
> Thanks,
> Andrei
>
> On Tue, Jun 11, 2019 at 11:24 AM Abhishek Dubey
> <dubeyabhishek777@gmail.com> wrote:
>> Please see attachment for Complete Console output.
>>
>> -Abhishek
>>
>> On Tue, Jun 11, 2019 at 11:52 PM Abhishek Dubey <dubeyabhishek777@gmail.com> wrote:
>>> Summary of test/zdtm.py run -a --ignore-taint --keep-going is
>>> ################## ALL TEST(S) PASSED (TOTAL 204/SKIPPED 21) ###################
>>>
>>> Sending complete console output separately.
>>>
>>> On Mon, Jun 10, 2019 at 10:50 PM Andrei Vagin <avagin@gmail.com> wrote:
>>>> On Mon, Jun 10, 2019 at 2:50 AM abhishek dubey
>>>> <dubeyabhishek777@gmail.com> wrote:
>>>>>
>>>>> On 10/06/19 12:59 PM, Andrei Vagin (C) wrote:
>>>>>> On Wed, Jun 05, 2019 at 12:49:03PM +0000, Pavel Emelianov wrote:
>>>>>>> On 6/4/19 7:50 PM, Abhishek Dubey wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
>>>>>>> Yes.
>>>>>> I don't understand what you mean here.
>>>>>>
>>>>>> https://ci.openvz.org/job/CRIU/job/CRIU-x86_64/job/criu-dev/5439/console
>>>>>>
>>>>>> Here we can see that all trasition tests were checked...
>>>>> Here I refer to locally running zdtm tests as given on
>>>>> https://www.criu.org/ZDTM_test_suite
>>>> Could you show output for zdtm.py run -a?
>>>>
>>>>>>>> What is specific about transition testcases?
>>>>>>> They take time to accomplish, so the default run is limited.
>>>>>>>
>>>>>>>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>>>>>>>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>>>>>>>> - Moved out unfreeze code from cr_pre_dump_finish()
>>>>>>>> - Moved out collect_mappings() from pre_dump_one_task
>>>>>>>>
>>>>>>>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>>>>>>>
>>>>>>>> Share your thoughts on this approach. I am attaching patch-file for reference.
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On running "zdtm.py run -a", testcases of zdtm/static directory are checked and that of zdtm/transition are skipped. Do all testcases of transition directory need manual run?
>>>>>>>> What is specific about transition testcases?
>>>>>>>>
>>>>>>>> For the first step of fetching VMAs between freeze and unfreeze of pre-dump:
>>>>>>>> - To maintain correspondence between PID and collected VMA, added new entry in struct pstree_item
>>>>>>>> - Moved out unfreeze code from cr_pre_dump_finish()
>>>>>>>> - Moved out collect_mappings() from pre_dump_one_task
>>>>>>> Please, describe why you need this. Current code does "fetching vmas between freeze and unfreeze",
>>>>>>> what's wrong with it so you need this change?
>>>>>>>
>>>>>>>> I have tested these changes with env00-basic example and zdtm.py test-suite. All test-cases under zdtm/static directory are Passing. Remaining transition test cases are skipped.
>>>>>>>>
>>>>>>>> Share your thoughts on this approach. I am attaching patch-file for reference.
>>>>>>>>
>>>>>>>> -Abhishek
>>>>>>>>
>>>>>>>> 0001-freezing-target-task-to-collect-memory-mappings-foll.patch
>>>>>>>>
>>>>>>>>   From 5b131559ce20ebbdb8a04be2cced321b55d864f4 Mon Sep 17 00:00:00 2001
>>>>>>> Please mind the https://criu.org/How_to_submit_patches article. Patch should go inline, not attached,
>>>>>>> and properly formatted. Also comments are inline.
>>>>>>>
>>>>>>>> From: abhidubey4178 <abhishekdubey4178940@gmail.com>
>>>>>>>> Date: Tue, 4 Jun 2019 19:59:21 +0530
>>>>>>>> Subject: [PATCH] freezing target task to collect memory mappings, followed by
>>>>>>>>    unfreeze
>>>>>>>>
>>>>>>>> ---
>>>>>>>>    criu/cr-dump.c        | 103 +++++++++++++++++++++++++++++++-----------
>>>>>>>>    criu/include/pstree.h |   1 +
>>>>>>>>    2 files changed, 78 insertions(+), 26 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
>>>>>>>> index 7f2e5edf..40bc7594 100644
>>>>>>>> --- a/criu/cr-dump.c
>>>>>>>> +++ b/criu/cr-dump.c
>>>>>>>> @@ -1154,20 +1154,19 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>>>>>>>      if (item->pid->state == TASK_DEAD)
>>>>>>>>              return 0;
>>>>>>>>
>>>>>>>> -   ret = collect_mappings(pid, &vmas, NULL);
>>>>>>> Up above there have been left the now unused vmas variable.
>>>>>>>
>>>>>>>> -   if (ret) {
>>>>>>>> +   if (!item->vmas) {
>>>>>>>>              pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>>>>>>>>              goto err;
>>>>>>>>      }
>>>>>>>>
>>>>>>>>      ret = -1;
>>>>>>>> -   parasite_ctl = parasite_infect_seized(pid, item, &vmas);
>>>>>>>> +   parasite_ctl = parasite_infect_seized(pid, item, item->vmas);
>>>>>>>>      if (!parasite_ctl) {
>>>>>>>>              pr_err("Can't infect (pid: %d) with parasite\n", pid);
>>>>>>>>              goto err_free;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> -   ret = parasite_fixup_vdso(parasite_ctl, pid, &vmas);
>>>>>>>> +   ret = parasite_fixup_vdso(parasite_ctl, pid, item->vmas);
>>>>>>>>      if (ret) {
>>>>>>>>              pr_err("Can't fixup vdso VMAs (pid: %d)\n", pid);
>>>>>>>>              goto err_cure;
>>>>>>>> @@ -1192,14 +1191,14 @@ static int pre_dump_one_task(struct pstree_item *item, InventoryEntry *parent_ie
>>>>>>>>      mdc.stat = NULL;
>>>>>>>>      mdc.parent_ie = parent_ie;
>>>>>>>>
>>>>>>>> -   ret = parasite_dump_pages_seized(item, &vmas, &mdc, parasite_ctl);
>>>>>>>> +   ret = parasite_dump_pages_seized(item, item->vmas, &mdc, parasite_ctl);
>>>>>>>>      if (ret)
>>>>>>>>              goto err_cure;
>>>>>>>>
>>>>>>>>      if (compel_cure_remote(parasite_ctl))
>>>>>>>>              pr_err("Can't cure (pid: %d) from parasite\n", pid);
>>>>>>>>    err_free:
>>>>>>>> -   free_mappings(&vmas);
>>>>>>>> +   free_mappings(item->vmas);
>>>>>>>>    err:
>>>>>>>>      return ret;
>>>>>>>>
>>>>>>>> @@ -1471,26 +1470,9 @@ static int setup_alarm_handler()
>>>>>>>>
>>>>>>>>    static int cr_pre_dump_finish(int status)
>>>>>>>>    {
>>>>>>>> -   InventoryEntry he = INVENTORY_ENTRY__INIT;
>>>>>>>>      struct pstree_item *item;
>>>>>>>>      int ret;
>>>>>>>>
>>>>>>>> -   /*
>>>>>>>> -    * Restore registers for tasks only. The threads have not been
>>>>>>>> -    * infected. Therefore, the thread register sets have not been changed.
>>>>>>>> -    */
>>>>>>>> -   ret = arch_set_thread_regs(root_item, false);
>>>>>>>> -   if (ret)
>>>>>>>> -           goto err;
>>>>>>>> -
>>>>>>>> -   ret = inventory_save_uptime(&he);
>>>>>>>> -   if (ret)
>>>>>>>> -           goto err;
>>>>>>>> -
>>>>>>>> -   pstree_switch_state(root_item, TASK_ALIVE);
>>>>>>>> -
>>>>>>>> -   timing_stop(TIME_FROZEN);
>>>>>>>> -
>>>>>>>>      if (status < 0) {
>>>>>>>>              ret = status;
>>>>>>>>              goto err;
>>>>>>>> @@ -1540,9 +1522,6 @@ err:
>>>>>>>>      if (bfd_flush_images())
>>>>>>>>              ret = -1;
>>>>>>>>
>>>>>>>> -   if (write_img_inventory(&he))
>>>>>>>> -           ret = -1;
>>>>>>>> -
>>>>>>> You write inventory too early, I'm not 100% sure this is correct.
>>>>>>>
>>>>>>>>      if (ret)
>>>>>>>>              pr_err("Pre-dumping FAILED.\n");
>>>>>>>>      else {
>>>>>>>> @@ -1552,6 +1531,68 @@ err:
>>>>>>>>      return ret;
>>>>>>>>    }
>>>>>>>>
>>>>>>>> +
>>>>>>>> +static int pre_dump_collect_vmas(struct pstree_item *item)
>>>>>>>> +{
>>>>>>>> +       pid_t pid = item->pid->real;
>>>>>>>> +       struct vm_area_list *vmas;
>>>>>>>> +       int ret = -1;
>>>>>>>> +
>>>>>>>> +       vmas = xmalloc(sizeof(struct vm_area_list));
>>>>>>> This memory is never free-d.
>>>>>>>
>>>>>>>> +       if(!vmas)
>>>>>>>> +            return -1;
>>>>>>>> +       INIT_LIST_HEAD(&vmas->h);
>>>>>>>> +       vmas->nr = 0;
>>>>>>>> +
>>>>>>>> +       pr_info("========================================\n");
>>>>>>>> +       pr_info("Collecting VMAs of (pid: %d)\n", pid);
>>>>>>>> +       pr_info("========================================\n");
>>>>>>>> +
>>>>>>>> +       if (item->pid->state == TASK_STOPPED) {
>>>>>>>> +               pr_warn("Stopped tasks are not supported\n");
>>>>>>>> +                    return 0;
>>>>>>>> +            }
>>>>>>> Formatting.
>>>>>>>
>>>>>>>> +
>>>>>>>> +       if (item->pid->state == TASK_DEAD)
>>>>>>>> +                    return 0;
>>>>>>> Vmas allocation can happen here.
>>>>>>>
>>>>>>>> +
>>>>>>>> +       ret = collect_mappings(pid, vmas, NULL);
>>>>>>>> +       if (ret) {
>>>>>>>> +               pr_err("Collect mappings (pid: %d) failed with %d\n", pid, ret);
>>>>>>>> +               return -1;
>>>>>>>> +            }
>>>>>>>> +
>>>>>>>> +       item->vmas = vmas;
>>>>>>>> +       return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +static int unfreeze_pstree()
>>>>>>>> +{
>>>>>>>> +   InventoryEntry he = INVENTORY_ENTRY__INIT;
>>>>>>>> +   int ret;
>>>>>>>> +
>>>>>>>> +   /*
>>>>>>>> +    * Restore registers for tasks only. The threads have not been
>>>>>>>> +    * infected. Therefore, the thread register sets have not been changed.
>>>>>>>> +    */
>>>>>>>> +   ret = arch_set_thread_regs(root_item, false);
>>>>>>>> +   if (ret)
>>>>>>>> +           return -1;
>>>>>>>> +
>>>>>>>> +   ret = inventory_save_uptime(&he);
>>>>>>>> +   if (ret)
>>>>>>>> +           return -1;
>>>>>>>> +
>>>>>>>> +   if (write_img_inventory(&he))
>>>>>>>> +           return -1;
>>>>>>>> +
>>>>>>>> +   pstree_switch_state(root_item, TASK_ALIVE);
>>>>>>>> +   timing_stop(TIME_FROZEN);
>>>>>>>> +
>>>>>>>> +   return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>    int cr_pre_dump_tasks(pid_t pid)
>>>>>>>>    {
>>>>>>>>      InventoryEntry *parent_ie = NULL;
>>>>>>>> @@ -1619,6 +1660,16 @@ int cr_pre_dump_tasks(pid_t pid)
>>>>>>>>      /* Errors handled later in detect_pid_reuse */
>>>>>>>>      parent_ie = get_parent_inventory();
>>>>>>>>
>>>>>>>> +   /* Collecting VMAs of all processes in pstree */
>>>>>>>> +   for_each_pstree_item(item)
>>>>>>>> +           if (pre_dump_collect_vmas(item))
>>>>>>>> +                   goto err;
>>>>>>>> +
>>>>>>>> +   /* Unfreeze pstree */
>>>>>>> This is pointless comment as it 100% repeats the subsequent function name :)
>>>>>>>
>>>>>>>> +   ret = unfreeze_pstree();
>>>>>>>> +   if(ret)
>>>>>>>> +           goto err;
>>>>>>>> +
>>>>>>>>      for_each_pstree_item(item)
>>>>>>>>              if (pre_dump_one_task(item, parent_ie))
>>>>>>>>                      goto err;
>>>>>>>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
>>>>>>>> index 7303c1fe..69562608 100644
>>>>>>>> --- a/criu/include/pstree.h
>>>>>>>> +++ b/criu/include/pstree.h
>>>>>>>> @@ -30,6 +30,7 @@ struct pstree_item {
>>>>>>>>              futex_t         task_st;
>>>>>>>>              unsigned long   task_st_le_bits;
>>>>>>>>      };
>>>>>>>> +   struct vm_area_list *vmas;
>>>>>>> This is dump-only field, it should be placed on dmp_info instead.
>>>>>>>
>>>>>>>>    };
>>>>>>>>
>>>>>>>>    static inline pid_t vpid(const struct pstree_item *i)
>>>>>>>> --
>>>>>>>> 2.17.1
>>>>>>>>
>>>>>>> -- Pavel
Pavel Emelianov June 20, 2019, 12:54 p.m.
On 6/20/19 1:00 AM, abhishek dubey wrote:
> Hi,
> 
> In the past week, I have been working on latest code to incorporate Andrei's attempt for using userspace buffer.
> 
> Additional changes:
> 
>         - skipping iovs generation for READ protected areas
> 
>         - skipping parasite injection (working on it)
> 
> 
> Debugging:
> 
> After implementing Andrei's changes and running zdtm suite, 28 test cases are failing. I am resolving this mismatch issue.
> 
> Error log of failed test:
> 
> === Run 324/362 ==============-- zdtm/static/different_creds
> ===================== Run zdtm/static/different_creds in h =====================
> Start test
> Test is SUID
> ./different_creds --pidfile=different_creds.pid --outfile=different_creds.out
> Run criu dump
> =[log]=> dump/zdtm/static/different_creds/38/1/dump.log
> ------------------------ grep Error ------------------------
> (00.007650) Fetched ack: 66 66 0
> (00.007655) Transferring pages:
> *(00.007658)     buf 2/1**
> **(00.007669) Error (criu/page-xfer.c:498): Can't splice (4096(8192)/2)*
> (00.007687) page-pipe: Killing page pipe
> (00.007727) ----------------------------------------
> (00.007732) Error (criu/mem.c:547): Can't dump page with parasite
> (00.008371) 38 was stopped
> (00.008487) Unlock network
> (00.008493) Unfreezing tasks into 1
> (00.008497)     Unseizing 38 into 1
> (00.008510) Error (criu/cr-dump.c:1753): Dumping FAILED.
> ------------------------ ERROR OVER ------------------------
> ############## Test zdtm/static/different_creds FAIL at CRIU dump ##############
> Send the 9 signal to  38
> Wait for zdtm/static/different_creds(38) to die for 0.100000
> 
> Queries:
> 
> 1) Whether zdtm test suite cover test cases to check functionality of pre-dump? Is there a particular test case which

It does, you need to use the --pre=1 option for that. Or --pre=2 :)

>      needs to be passed for pre-dump functionality verification?

Since you're touching the way criu tosses memory, you should also check that pre-dump is not broken.

> 2) Currently I am following steps below to check if pre-dump works fine (since some test cases which fails in zdtm suite passes when run this way):
> 
>         - make <testcase_name>.pid
> 
>         - predump <testcase>
> 
>         - dump <testcase> pointing to predump images
> 
>         - restore <testcase>
> 
>         - make <testcase_name>.out
> 
>         look for PASS message in last step.

That's correct and that's what --pre=<NUMBER> option does for you :)

-- Pavel