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

[Lai Jiangshan]

On Wed, Aug 10, 2016 at 5:03 PM, Juan Quintela <address@hidden> wrote:
> Lai Jiangshan <address@hidden> 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.