Re: [Qemu-devel] [PATCH 42/51] ram: Pass RAMBlock to bitmap_sync

[Yang Hongyang]

Hi Juan,

On 2017/3/24 16:29, Juan Quintela wrote:
> Yang Hongyang <address@hidden> wrote:
>> On 2017/3/24 4:45, Juan Quintela wrote:
>>> We change the meaning of start to be the offset from the beggining of
>>> the block.
>>>
>>> @@ -701,7 +701,7 @@ static void migration_bitmap_sync(RAMState *rs)
>>>      qemu_mutex_lock(&rs->bitmap_mutex);
>>>      rcu_read_lock();
>>>      QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>>> -        migration_bitmap_sync_range(rs, block->offset, block->used_length);
>>> +        migration_bitmap_sync_range(rs, block, 0, block->used_length);
>>
>> Since RAMBlock been passed to bitmap_sync, could we remove
>> param 'block->used_length' either?
> 
> Hi
> 
> good catch.
> 
> I had that removed, and then realized that I want to synchronize parts
> of the bitmap, not the whole one.  That part of the series is still not
> done.
> 
> Right now we do something like (I have simplified a lot of details):
> 
> while(true) {
>             foreach(block)
>                 bitmap_sync(block)
>             foreach(page)
>                 if(dirty(page))
>                    page_send(page)
> }
> 
> 
> If you have several terabytes of RAM that is too ineficient, because
> when we arrive to the page_send(page), it is possible that it is already
> dirty again, and we have to send it twice.  So, the idea is to change to
> something like:
> 
> while(true) {
>             foreach(block)
>                 bitmap_sync(block)

Do you mean sync with KVM here?

>             foreach(block)
>                 foreach(64pages)
>                     bitmap_sync(64pages)

Then here, we will sync with KVM too. For huge MEM,
it will generates lots of ioctl()...
Bitmap in KVM is per Memory region IIRC. KVM module currently
haven't the ability to sync parts of the bitmap. A sync have
to sync the whole mr. So if we want to do small sync, we might
need to modify KVM also, but that still won't solve the preblem
of increased ioctls.

>                     foreach(page of the 64)
>                        if (dirty)
>                           page_send(page)
> }
> 
> 
> Where 64 is a magic number, I have to test what is the good value.
> Basically it should be a multiple of sizeof(long) and a multiple/divisor
> of host page size.
> 
> Reason of changing the for to be for each block, is that then we can
> easily put bitmaps by hostpage size, instead of having to had it for
> target page size.
> 
> Thanks for the review, Juan.
> 
> Later, Juan.
> 

-- 
Thanks,
Yang