<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Please find Inline reply.<br>
<div class="moz-cite-prefix">On 05/06/19 6:19 PM, Pavel Emelianov
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">On 6/4/19 7:50 PM, Abhishek Dubey wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yes.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">What is specific about transition testcases?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
They take time to accomplish, so the default run is limited.</pre>
</blockquote>
So checking default is enough for patch submission?<br>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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?</pre>
</blockquote>
<p>In current approach TIME_FROZEN encapsulates TIME_MEMDUMP, like:</p>
<p><font size="-1"> <b> start(TIME_FROZEN)</b></font></p>
<p><font size="-1"><b> collect mappings</b></font></p>
<p><font size="-1"><b> start(TIME_MEMDUMP)</b></font></p>
<p><font size="-1"><b> step0: page map cache init</b></font></p>
<p><font size="-1"><b> step1: generate pagemap </b><b><br>
</b></font></p>
<p><font size="-1"><b> step2: grab pages into pipes</b></font></p>
<p><font size="-1"><b> end(TIME_MEMDUMP)</b></font></p>
<p><font size="-1"><b> unfreeze pstree<br>
</b></font></p>
<p><font size="-1"><b> end(TIME_FROZEN)</b></font><br>
</p>
<p>and the objective I perceived, is something like:</p>
<p> <font size="-1"><b>start(TIME_FROZEN) ----} </b><b>
</b></font></p>
<p><font size="-1"><b> collect mappings
}==> Current patch <br>
</b></font></p>
<p><font size="-1"><b> unfreeze pstree }<br>
</b></font></p>
<p><font size="-1"><b> </b><b>end(TIME_FROZEN) ----}</b></font>
</p>
<p><font size="-1"><b> start(TIME_MEMDUMP) </b></font></p>
<p><font size="-1"><b> step0: page map cache init</b></font></p>
<font size="-1"><b> </b></font>
<p><font size="-1"><b> step1: generate pagemap </b><b><br>
</b></font>
</p>
<font size="-1"><b> </b></font>
<p><font size="-1"><b> step2: grab pages into pipes</b></font></p>
<font size="-1"><b> </b></font>
<p><font size="-1"><b> end(TIME_MEMDUMP)</b></font></p>
<p><font size="-1"><b><br>
</b></font></p>
<p>With reference to the comment "fetching vmas between freeze and
unfreeze", I think this is what is needed:<br>
</p>
<p><font size="-1"><font size="-1"><b> start(TIME_FROZEN)</b></font>
</font></p>
<p><font size="-1"><b> collect mappings</b></font></p>
<p><font size="-1"><b> start(TIME_MEMDUMP)</b></font></p>
<p><font size="-1"><b> step0: page map cache init</b></font></p>
<p><font size="-1"><b> step1: generate pagemap </b><b><br>
</b></font></p>
<p><font size="-1"><b> end(TIME_MEMDUMP)</b></font></p>
<p><font size="-1"><b> unfreeze pstree<br>
</b></font></p>
<p><font size="-1"><b> end(TIME_FROZEN)</b></font></p>
<p><br>
<font size="-1"><b><font size="-1"><b> step2: grab pages into
user buffer using system_vm_readv and resolve races<br>
</b></font></b></font></p>
<p> Please confirm if above design is right?<font size="-1"><br>
</font></p>
<p><font size="-1"> So, I don't have to worry about unfreezing after
collection? Let it happen the way it is now.</font></p>
<p><font size="-1">My immediate task will be getting pipes out of
picture. I will start with </font><font size="-1">"commit
bb98a82" as reference, as <br>
</font></p>
<p><font size="-1">suggested by Radostin in other thread.</font></p>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Please mind the <a class="moz-txt-link-freetext" href="https://criu.org/How_to_submit_patches">https://criu.org/How_to_submit_patches</a> article. Patch should go inline, not attached,
and properly formatted. Also comments are inline.</pre>
</blockquote>
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.<br>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: abhidubey4178 <a class="moz-txt-link-rfc2396E" href="mailto:abhishekdubey4178940@gmail.com"><abhishekdubey4178940@gmail.com></a>
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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Up above there have been left the now unused vmas variable.</pre>
</blockquote>
ok.<br>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- 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;
-
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
You write inventory too early, I'm not 100% sure this is correct.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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));
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This memory is never free-d.</pre>
</blockquote>
Got it.<br>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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;
+ }
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Formatting.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ if (item->pid->state == TASK_DEAD)
+ return 0;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Vmas allocation can happen here.</pre>
</blockquote>
Got it.<br>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ 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 */
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This is pointless comment as it 100% repeats the subsequent function name :)</pre>
</blockquote>
Won't repeat!<br>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This is dump-only field, it should be placed on dmp_info instead.</pre>
</blockquote>
Sure. <br>
<blockquote type="cite"
cite="mid:fff380f4-acd3-ca37-e261-b08c1aa24a69@virtuozzo.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> };
static inline pid_t vpid(const struct pstree_item *i)
--
2.17.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
-- Pavel
</pre>
</blockquote>
</body>
</html>