On Wed, Jul 07, 2021 at 08:34:50AM +0000, Wang, Wei W wrote:
> On Wednesday, July 7, 2021 1:47 AM, Peter Xu wrote:
> > On Sat, Jul 03, 2021 at 02:53:27AM +0000, Wang, Wei W wrote:
> > > + do {
> > > + page_to_clear = start + (i++ << block->clear_bmap_shift);
> >
> > Why "i" needs to be shifted?
>
> Just move to the next clear chunk, no?
> For example, (1 << 18) pages chunk (i.e. 1GB).
But migration_clear_memory_region_dirty_bitmap() has done the shifting?
>
> >
> > > + migration_clear_memory_region_dirty_bitmap(ram_state,
> > > + block,
> > > +
> > page_to_clear);
> > > + } while (i <= npages >> block->clear_bmap_shift);
> >
> > I agree with David that this should be better put into the mutex section,
> > if so
> > we'd also touch up comment for bitmap_mutex. Or is it a reason to
> > explicitly
> > not do so?
>
> clear_bmap_test_and_clear already uses atomic operation on clear_bmap.
> But it's also OK to me if you guys feel safer to move it under the lock.
I see, yes seems ok to be out of the lock. Or we use mutex to protect all of
them, then make clear_bmap* helpers non-atomic too, just like dirty bmap.
Thanks,
--
Peter Xu
