Hi,
Any update?
Thanks,
Zhang Haoyu
On 2016/8/30 12:11, Lai Jiangshan wrote:
> On Wed, Aug 10, 2016 at 5:03 PM, Juan Quintela <quint...@redhat.com> wrote:
>> Lai Jiangshan <jiangshan...@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.
>
