[CRIU] GSoC: Memory mapping collection
abhishek dubey
dubeyabhishek777 at gmail.com
Mon Jun 10 12:51:30 MSK 2019
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 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