[CRIU] GSoC: Memory mapping collection
Pavel Emelianov
xemul at virtuozzo.com
Wed Jun 5 15:49:03 MSK 2019
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 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