Re: [PATCH v1] migration: clear the memory region dirty bitmap when skipping free pages

[Peter Xu]

On Wed, Jul 14, 2021 at 02:58:31PM +0000, Wang, Wei W wrote:
> On Wednesday, July 14, 2021 6:30 PM, David Hildenbrand wrote:
> > 
> > On 14.07.21 12:27, Michael S. Tsirkin wrote:
> > > On Wed, Jul 14, 2021 at 03:51:04AM -0400, Wei Wang wrote:
> > >> When skipping free pages, their corresponding dirty bits in the
> > >> memory region dirty bitmap need to be cleared. Otherwise the skipped
> > >> pages will be sent in the next round after the migration thread syncs
> > >> dirty bits from the memory region dirty bitmap.
> > >>
> > >> migration_clear_memory_region_dirty_bitmap_range is put outside the
> > >> bitmap_mutex, becasue
> > >
> > > because?
> > >
> > >> memory_region_clear_dirty_bitmap is possible to block on the kvm slot
> > >> mutex (don't want holding bitmap_mutex while blocked on another
> > >> mutex), and clear_bmap_test_and_clear uses atomic operation.
> > 
> > How is that different from our existing caller?
> > 
> > Please either clean everything up, completely avoiding the lock (separate
> > patch), or move it under the lock.
> > 
> > Or am I missing something important?
> 
> That seems ok to me and Peter to have it outside the lock. Not sure if Dave 
> or Juan knows the reason why clear_bmap needs to be under the mutex given 
> that it is atomic operation.

Yes it looks ok to not have the lock to me, but I still think it's easier to
put all bitmap ops under the bitmap_mutex, so we handle clear_bmap/bmap the
same way.  It's also what we did in the existing code (although by accident).

Then we can replace clear_bmap atomic ops to normal mem accesses in a follow up
patch.  But it won't affect a huge lot - unlike normal bmap, clear_bmap is
normally per 1g chunk so modifying clear_bmap happens much less frequently.

Atomic ops will be needed of course if we want a spinlock version of
bitmap_mutex, however I still don't know whether that'll really help anything.

Thanks,

-- 
Peter Xu