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.