On 01/12/2017 08:44 PM, Lai Jiangshan wrote: > On Fri, Jan 13, 2017 at 3:19 AM, Jianjun Duan <du...@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 <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. >>> >> >