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