[CRIU] GSoC: Memory mapping collection
Andrei Vagin
avagin at gmail.com
Mon Jun 10 20:20:10 MSK 2019
On Mon, Jun 10, 2019 at 2:50 AM abhishek dubey
<dubeyabhishek777 at 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 at 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
More information about the CRIU
mailing list