Re: [PATCH] migration: Move bitmap_mutex out of migration_bitmap_clear_dirty()

[Peter Xu]

On Tue, Jul 06, 2021 at 12:05:49PM +0200, David Hildenbrand wrote:
> On 06.07.21 11:41, Wang, Wei W wrote:
> > On Monday, July 5, 2021 9:42 PM, David Hildenbrand wrote:
> > > On 03.07.21 04:53, Wang, Wei W wrote:
> > > > On Friday, July 2, 2021 3:07 PM, David Hildenbrand wrote:
> > > > > On 02.07.21 04:48, Wang, Wei W wrote:
> > > > > > On Thursday, July 1, 2021 10:22 PM, David Hildenbrand wrote:
> > > > > > > On 01.07.21 14:51, Peter Xu wrote:
> > > > > 
> > > > > I think that clearly shows the issue.
> > > > > 
> > > > > My theory I did not verify yet: Assume we have 1GB chunks in the 
> > > > > clear bmap.
> > > > > Assume the VM reports all pages within a 1GB chunk as free (easy with
> > > > > a fresh VM). While processing hints, we will clear the bits from the
> > > > > dirty bmap in the RAMBlock. As we will never migrate any page of that
> > > > > 1GB chunk, we will not actually clear the dirty bitmap of the memory
> > > > > region. When re-syncing, we will set all bits bits in the dirty bmap
> > > > > again from the dirty bitmap in the memory region. Thus, many of our
> > > > > hints end up being mostly ignored. The smaller the clear bmap chunk, 
> > > > > the
> > > more extreme the issue.
> > > > 
> > > > OK, that looks possible. We need to clear the related bits from the
> > > > memory region bitmap before skipping the free pages. Could you try with
> > > below patch:
> > > 
> > > I did a quick test (with the memhog example) and it seems like it mostly 
> > > works.
> > > However, we're now doing the bitmap clearing from another, racing with the
> > > migration thread. Especially:
> > > 
> > > 1. Racing with clear_bmap_set() via 
> > > cpu_physical_memory_sync_dirty_bitmap()
> > > 2. Racing with migration_bitmap_clear_dirty()
> > > 
> > > So that might need some thought, if I'm not wrong.
> > 
> > I think this is similar to the non-clear_bmap case, where the rb->bmap is 
> > set or cleared by
> > the migration thread and qemu_guest_free_page_hint. For example, the 
> > migration thread
> > could find a bit is set from rb->bmap before qemu_guest_free_page_hint gets 
> > that bit cleared in time.
> > The result is that the free page is transferred, which isn't necessary, but 
> > won't cause any issue.
> > This is very rare in practice.
> 
> Here are my concerns regarding races:
> 
> a) We now end up calling migration_clear_memory_region_dirty_bitmap()
> without holding the bitmap_mutex. We have to clarify if that is ok. At least
> migration_bitmap_clear_dirty() holds it *while* clearing the log and
> migration_bitmap_sync() while setting bits in the clear_bmap. I think we
> also have to hold the mutex on the new path.

Makes sense; I think we can let bitmap_mutex to protect both dirty/clear
bitmaps, and also the dirty pages counter.  I'll comment in Wei's patch too 
later.

> 
> b) We now can end up calling memory_region_clear_dirty_bitmap()
> concurrently, not sure if all notifiers can handle that. I'd assume it is
> okay, but I wouldn't bet on it.

Yes, kvm should be the only one that uses it for real, while there's the slots
lock in kvm_physical_log_clear(), looks okay so far.

We'd better not turn off clear dirty log for this; it's a feature that I know
brought lots of benefits to get dirty logging, and we're even considering to
enable it for dirty ring so dirty ring can also benefit from it.

Thanks,

-- 
Peter Xu