Re: [Qemu-devel] [PATCH] migratioin/ram: leave RAMBlock->bmap blank on allocating

[Wei Yang <richardw.yang@linux.intel.com>]

On Mon, Jun 03, 2019 at 10:35:27AM +0800, Peter Xu wrote:
>On Mon, Jun 03, 2019 at 09:33:05AM +0800, Wei Yang wrote:
>> On Sat, Jun 01, 2019 at 11:34:41AM +0800, Peter Xu wrote:
>> >On Fri, May 31, 2019 at 05:43:37PM +0100, Dr. David Alan Gilbert wrote:
>> >> * Wei Yang (richardw.yang@linux.intel.com) wrote:
>> >> > During migration, we would sync bitmap from ram_list.dirty_memory to
>> >> > RAMBlock.bmap in cpu_physical_memory_sync_dirty_bitmap().
>> >> > 
>> >> > Since we set RAMBlock.bmap and ram_list.dirty_memory both to all 1, this
>> >> > means at the first round this sync is meaningless and is a duplicated
>> >> > work.
>> >> > 
>> >> > Leaving RAMBlock->bmap blank on allocating would have a side effect on
>> >> > migration_dirty_pages, since it is calculated from the result of
>> >> > cpu_physical_memory_sync_dirty_bitmap(). To keep it right, we need to
>> >> > set migration_dirty_pages to 0 in ram_state_init().
>> >> > 
>> >> > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> 
>> >> I've looked at this for a while, and I think it's OK, so
>> >> 
>> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >> 
>> >> Peter, Juan: Can you just see if there's arny reason this would be bad,
>> >> but I think it's actually more sensible than what we have.
>> >
>> >I really suspect it will work in all cases...  Wei, have you done any
>> >test (or better, thorough tests) with this change?  My reasoning of
>> >why we should need the bitmap all set is here:
>> >
>> 
>> I have done some migration cases, like migrate a linux guest through tcp.
>
>When did you start the migration?  Have you tried to migrate during
>some workload?
>

I tried kernel build in guest.

>> 
>> Other cases suggested to do?
>
>Could you also help answer the question I raised below in the link?
>
>Thanks,
>
>> >https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg07361.html
>

I took a look into this link, hope my understanding is correct.

You concern is this thread/patch is based on one prerequisite -- dirty all the
bitmap at start.

My answer is we already did it in ram_block_add() for each RAMBlock. And then
the bitmap is synced by migration_bitmap_sync_precopy() from
ram_list.dirty_memory to RAMBlock.bmap.


>-- 
>Peter Xu

-- 
Wei Yang
Help you, Help me