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

[Alexander Graf <agraf@suse.de>]

Juan,

It looks like Lai is waiting for a reply from you on this email :).


Alex

On 08/30/2016 06:11 AM, 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.