Re: [Qemu-devel] [PATCH v4 2/4] migration: API to clear bits of guest free pages from the dirty bitmap

[Wei Wang <wei.w.wang@intel.com>]

On 03/15/2018 03:42 AM, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> On Wed, Mar 14, 2018 at 06:11:37PM +0000, Dr. David Alan Gilbert wrote:
>>>> +            used_len = block->used_length - offset;
>>>> +            addr += used_len;
>>>> +        }
>>>> +
>>>> +        start = offset >> TARGET_PAGE_BITS;
>>>> +        npages = used_len >> TARGET_PAGE_BITS;
>>>> +        ram_state->migration_dirty_pages -=
>>>> +                      bitmap_count_one_with_offset(block->bmap, start, npages);
>>>> +        bitmap_clear(block->bmap, start, npages);
>>> If this is happening while the migration is running, this isn't safe -
>>> the migration code could clear a bit at about the same point this
>>> happens, so that the count returned by bitmap_count_one_with_offset
>>> wouldn't match the word that was cleared by bitmap_clear.
>>>
>>> The only way I can see to fix it is to run over the range using
>>> bitmap_test_and_clear_atomic, using the return value to decrement
>>> the number of dirty pages.
>>> But you also need to be careful with the update of the
>>> migration_dirty_pages value itself, because that's also being read
>>> by the migration thread.
>>>
>>> Dave
>> I see that there's migration_bitmap_sync but it does not seem to be
> Do you mean bitmap_mutex?
>
>> taken on all paths. E.g. migration_bitmap_clear_dirty and
>> migration_bitmap_find_dirty are called without that lock sometimes.
>> Thoughts?

Right. The bitmap claims to protect modification of the bitmap, but 
migration_bitmap_clear_dirty doesn't strictly follow the rule.

> Hmm, that doesn't seem to protect much at all!  It looks like it was
> originally added to handle hotplug causing the bitmaps to be resized;
> that extension code was removed in 66103a5 so that lock can probably go.
>
> I don't see how the lock would help us though; the migration thread is
> scanning it most of the time so would have to have the lock held
> most of the time.
>



How about adding the lock to migration_bitmap_clear_dirty, and we will 
have something like this:

migration_bitmap_clear_dirty()
{
     qemu_mutex_lock(&rs->bitmap_mutex);
     ret = test_and_clear_bit(page, rb->bmap);
      if (ret) {
         rs->migration_dirty_pages--;
     }
     ...
     qemu_mutex_unlock(&rs->bitmap_mutex);
}


qemu_guest_free_page_hint()
{
     qemu_mutex_lock(&rs->bitmap_mutex);
     ...
     ram_state->migration_dirty_pages -=
                       bitmap_count_one_with_offset(block->bmap, start, 
npages);
     bitmap_clear(block->bmap, start, npages);
     qemu_mutex_unlock(&rs->bitmap_mutex);
}


The migration thread will hold the lock only when it clears a bit from 
the bitmap. Or would you consider to change it to qemu_spin_lock?

Best,
Wei