Re: [Qemu-devel] [PATCH] migratioin/ram.c: reset complete_round when we gets a queued page

[Wei Yang <richardw.yang@linux.intel.com>]

On Wed, Jun 05, 2019 at 02:41:08PM +0800, Peter Xu wrote:
>On Wed, Jun 05, 2019 at 09:08:28AM +0800, Wei Yang wrote:
>> In case we gets a queued page, the order of block is interrupted. We may
>> not rely on the complete_round flag to say we have already searched the
>> whole blocks on the list.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/ram.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index d881981876..e9b40d636d 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2290,6 +2290,12 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
>>           */
>>          pss->block = block;
>>          pss->page = offset >> TARGET_PAGE_BITS;
>> +
>> +        /*
>> +         * This unqueued page would break the "one round" check, even is
>> +         * really rare.
>
>Why this is needed?  Could you help explain the problem first?

Peter, Thanks for your question.

I found this issue during code review and I believe this is a corner case.

Below is a draft chart for ram_find_and_save_block:

    ram_find_and_save_block
        do
            get_queued_page()
            find_dirty_block()
            ram_save_host_page()
        while

The basic logic here is : get a page need to migrate and migrate it.

In case we don't have get_queued_page(), find_dirty_block() will search the
whole ram_list.blocks by order. pss->complete_round is used to indicate
whether this search has looped.

Everything works fine after get_queued_page() involved. The block unqueued in
get_queued_page() could be any block in the ram_list.blocks. This means we
have very little chance to break the looped indicator.

                           unqueue_page()  last_seen_block
                                     |     |
    ram_list.blocks                  v     v
    ---------------------------------+=====+---


Just draw a raw picture to demonstrate a corner case.

For example, we start from last_seen_block and search till the end of
ram_list.blocks. At this moment, pss->complete_round is set to true. Then we
get a queued page from unqueue_page() at the point I pointed. So the loop
continues may just continue the range as I marked as "=". We will skip all the
other ranges.

This is really a corner case, since ram_save_host_page() should return 0 and
there should be no dirty page in this range. But I don't see we may avoid this
case.

If I am not correct, just let me know :-)

>
>Thanks,
>
>-- 
>Peter Xu

-- 
Wei Yang
Help you, Help me