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

[Peter Xu]

On Mon, Jun 03, 2019 at 11:36:00AM +0800, Wei Yang wrote:
> 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 (address@hidden) 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 <address@hidden>
> >> >> 
> >> >> I've looked at this for a while, and I think it's OK, so
> >> >> 
> >> >> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> >> >> 
> >> >> 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.

Ah I see, thanks for the pointer.  Then I would agree it's fine.

I'm not an expert of TCG - I'm curious on why all those three dirty
bitmaps need to be set at the very beginning.  IIUC at least the VGA
bitmap should not require that (so IMHO we should be fine to have all
zeros with VGA bitmap for ramblocks, and we only set them when the
guest touches them).  Migration bitmap should be special somehow but I
don't know much on TCG/TLB part I'd confess so I can't say.  In other
words, if migration is the only one that requires this "all-1"
initialization then IMHO we may consider to remove the other part
rather than here in migration because that's what we'd better to be
sure with.

And even if you want to remove this, I still have two suggestions:

(1) proper comment here above bmap on the above fact that although
    bmap is not set here but it's actually set somewhere else because
    we'll sooner or later copy all 1s from the ramblock bitmap

(2) imho you can move "migration_dirty_pages = 0" into
    ram_list_init_bitmaps() too to let them be together

-- 
Peter Xu