Re: [Qemu-devel] [PATCH 30/30] ram: optimize migration bitmap walking

[Orit Wasserman]

On 10/28/2012 10:35 AM, Orit Wasserman wrote:
> On 10/26/2012 01:39 PM, Juan Quintela wrote:
>> Orit Wasserman <address@hidden> wrote:
>>> On 10/18/2012 09:30 AM, Juan Quintela wrote:
>>>> Instead of testing each page individually, we search what is the next
>>>> dirty page with a bitmap operation.  We have to reorganize the code to
>>>> move from a "for" loop, to a while(dirty) loop.
>>>>
>>
>>>>
>>>> -    do {
>>>> +    while(true) {
>>>>          mr = block->mr;
>>>> -        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
>>>> +        offset = migration_bitmap_find_and_reset_dirty(mr, offset);
>>>> +        if (complete_round && block == last_seen_block &&
>>>> +            offset >= last_offset) {
>>>> +            break;
>>>> +        }
>>> Juan,
>>> You need to exchange those line, first check to see you did a full round 
>>> than
>>> calculate and reset the offset, in the way it is written now you may
>>> reset a bit and than break of the loop
>>> without sending it.
>>
>> How?
>>
>> if complete_round == true, it means that we are in the second round.
>>
>> block == last_seen_block means that we are back at the 1st block that we
>> have looked.
>>
>> if offset >= last_offset, there are two options:
>> a- == last_offset: that was the 1st one that we checked, so it can't be
>>    true.
>> b- >= last_offset: it means tat we have already passed that bit, it
>>    _has_ to be zero, otherwise somebody has changed the bitmap under or
>>    foot.
>>
>> Or have I missed something?
> If we are in iterate state this means the bitmap is changing all the time,
> even when we didn't finish a complete cycle (for example we get to the 
> bandwidth limit, exit ram_save_iterate and sync the bitmap in pending).
> This means that bits in part of the bitmap we already walked can get dirty 
> again. 
> In that case the if condition can be true for a dirty bit,
> in the current code we reset it and than break for the loop, this means it is 
> set clean without sending it, which is a problem.
> Changing the line order will fix it (the way it was before).
> 
OK read the code more thoroughly, there is no way that the bitmap will if we do 
a full cycle in ram_save_block,
We can actually simplify and  the function by checking the number of dirty 
pages. If it is zero we can return 
immediately.
If it is not zero it means we will exit the loop when we will find the dirty 
page.

Cheers,
Orit 
> Regards,
> Orit
> 
>> Notice that at some point we should allow for concurrent dirty of the
>> bitmap, but we need to do yet more things.
>>
>> Later, Juan.
>>
> 
>