Re: [Qemu-devel] [PATCH] add migration capability to bypass the shared memory

[Jianjun Duan <duanj@linux.vnet.ibm.com>]

On 01/12/2017 08:44 PM, Lai Jiangshan wrote:
> On Fri, Jan 13, 2017 at 3:19 AM, Jianjun Duan <duanj@linux.vnet.ibm.com> wrote:
>> I have a question related to interplay of bypassing the shared memory in
>> migration and memory hotplugging. If on the source guest a big chunk of
>> memory is plugged in, will the shared memory still be mapped the same
>> way on the guest? i.e, the mapping from guest physical address to the
>> host virtual address be the same?
> 
> I don't understand the question, the patch doesn't change
> the memory hotplugging nor the way how the pages are mapped
> in the guest physical.
> 
Let me try to rephrase it. Is there a scenario in which the mapping
could change? If the mapping could change, will bypassing shared memory
still work?

Thanks,
Jianjun

>>
>> Thanks,
>> Jianjun
>>
>>
>> On 08/29/2016 09:11 PM, Lai Jiangshan wrote:
>>> On Wed, Aug 10, 2016 at 5:03 PM, Juan Quintela <quintela@redhat.com> wrote:
>>>> Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>>>>
>>>> Hi
>>>>
>>>> First of all, I like a lot the patchset, but I would preffer to split it
>>>> to find "possible" bugs along the lines, especially in postcopy, but not only.
>>>
>>> Hello, thanks for review and comments
>>>
>>> I tried to make the patch be sane and tight.
>>> I don't see any strong reason to split it without complicating the patch.
>>>
>>>>
>>>> [very nice description of the patch]
>>>>
>>>> Nothing to say about the QMP and shared memory detection, looks correct
>>>> to me.
>>>>
>>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>>> index 815bc0e..880972d 100644
>>>>> --- a/migration/ram.c
>>>>> +++ b/migration/ram.c
>>>>> @@ -605,6 +605,28 @@ static void migration_bitmap_sync_init(void)
>>>>>      num_dirty_pages_period = 0;
>>>>>      xbzrle_cache_miss_prev = 0;
>>>>>      iterations_prev = 0;
>>>>> +    migration_dirty_pages = 0;
>>>>> +}
>>>>> +
>>>>> +static void migration_bitmap_init(unsigned long *bitmap)
>>>>> +{
>>>>> +    RAMBlock *block;
>>>>> +
>>>>> +    bitmap_clear(bitmap, 0, last_ram_offset() >> TARGET_PAGE_BITS);
>>>>> +    rcu_read_lock();
>>>>> +    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>>>>> +        if (!migrate_bypass_shared_memory() || !qemu_ram_is_shared(block)) {
>>>>> +            bitmap_set(bitmap, block->offset >> TARGET_PAGE_BITS,
>>>>> +                       block->used_length >> TARGET_PAGE_BITS);
>>>>> +
>>>>> +            /*
>>>>> +             * Count the total number of pages used by ram blocks not including
>>>>> +             * any gaps due to alignment or unplugs.
>>>>> +             */
>>>>> +         migration_dirty_pages += block->used_length >> TARGET_PAGE_BITS;
>>>>> +     }
>>>>> +    }
>>>>> +    rcu_read_unlock();
>>>>>  }
>>>>
>>>> We can split this function in a different patch.
>>>
>>> it calls the new function migrate_bypass_shared_memory().
>>> it is no a good idea to split it out.
>>>
>>>> I haven't fully search
>>>> if we care about taking the rcu lock here.  The thing that I am more
>>>> interested is in knowing what happens when we don't set
>>>> migration_dirty_pages as the full "possible" memory pages.
>>>
>>> I hadn't tested it with postcopy, I don't know how to use postcopy.
>>> From my review I can't find obvious bugs about it.
>>>
>>> I don't think there is any good reason to use migrate_bypass
>>> and postcopy together,  I can disable the migrate_bypass
>>> when postcopy==true if you want.
>>>
>>>>
>>>> Once here, should we check for ROM regions?
>>>>
>>>> BTW, could'nt we use:
>>>>
>>>> int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
>>>> {
>>>>     RAMBlock *block;
>>>>     int ret = 0;
>>>>
>>>>     rcu_read_lock();
>>>>     QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>>>>         ret = func(block->idstr, block->host, block->offset,
>>>>                    block->used_length, opaque);
>>>>         if (ret) {
>>>>             break;
>>>>         }
>>>>     }
>>>>     rcu_read_unlock();
>>>>     return ret;
>>>> }
>>>>
>>>
>>> the patch only introduces only one "QLIST_FOREACH_RCU(ram_list.blocks)"
>>> but
>>> # git grep 'QLIST_FOREACH_RCU.*ram_list'  | wc -l
>>> #       16
>>>
>>> I don't want to introduce qemu_ram_foreach_block()
>>> and touch another 15 places.
>>> I hope someone do it after merged.
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>  static void migration_bitmap_sync(void)
>>>>> @@ -631,7 +653,9 @@ static void migration_bitmap_sync(void)
>>>>>      qemu_mutex_lock(&migration_bitmap_mutex);
>>>>>      rcu_read_lock();
>>>>>      QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>>>>> -        migration_bitmap_sync_range(block->offset, block->used_length);
>>>>> +        if (!migrate_bypass_shared_memory() || !qemu_ram_is_shared(block)) {
>>>>> +            migration_bitmap_sync_range(block->offset, block->used_length);
>>>>> +        }
>>>>>      }
>>>>>      rcu_read_unlock();
>>>>>      qemu_mutex_unlock(&migration_bitmap_mutex);
>>>>
>>>> Oops, another place where we were not using qemu_ram_foreach_block :p
>>>>
>>>>
>>>>> @@ -1926,19 +1950,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>>>>      ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
>>>>>      migration_bitmap_rcu = g_new0(struct BitmapRcu, 1);
>>>>>      migration_bitmap_rcu->bmap = bitmap_new(ram_bitmap_pages);
>>>>> -    bitmap_set(migration_bitmap_rcu->bmap, 0, ram_bitmap_pages);
>>>>> +    migration_bitmap_init(migration_bitmap_rcu->bmap);
>>>>>
>>>>>      if (migrate_postcopy_ram()) {
>>>>>          migration_bitmap_rcu->unsentmap = bitmap_new(ram_bitmap_pages);
>>>>> -        bitmap_set(migration_bitmap_rcu->unsentmap, 0, ram_bitmap_pages);
>>>>> +        bitmap_copy(migration_bitmap_rcu->unsentmap,
>>>>> +                 migration_bitmap_rcu->bmap, ram_bitmap_pages);
>>>>>      }
>>>>
>>>> I think that if we go this route, we should move the whole if inside the
>>>> migration_bitmap_init?
>>>
>>> good! I will do it when I update the patch.
>>>
>>> Thanks,
>>> Lai
>>>
>>>>
>>>>>
>>>>> -    /*
>>>>> -     * Count the total number of pages used by ram blocks not including any
>>>>> -     * gaps due to alignment or unplugs.
>>>>> -     */
>>>>> -    migration_dirty_pages = ram_bytes_total() >> TARGET_PAGE_BITS;
>>>>> -
>>>>>      memory_global_dirty_log_start();
>>>>>      migration_bitmap_sync();
>>>>>      qemu_mutex_unlock_ramlist();
>>>>
>>>>
>>>> As said, very happy with the patch.  And it got much simpler that I
>>>> would have expected.
>>>>
>>>> Thanks, Juan.
>>>
>>
>