Re: [Qemu-devel] [PATCH v5 1/4] migration: do not flush_compressed_data at the end of each iteration

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 09/04/2018 12:38 AM, Juan Quintela wrote:
> guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> flush_compressed_data() needs to wait all compression threads to
>> finish their work, after that all threads are free until the
>> migration feeds new request to them, reducing its call can improve
>> the throughput and use CPU resource more effectively
>> We do not need to flush all threads at the end of iteration, the
>> data can be kept locally until the memory block is changed or
>> memory migration starts over in that case we will meet a dirtied
>> page which may still exists in compression threads's ring
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> I am not so sure about this patch.
> 
> Right now, we warantee that after each iteration, all data is written
> before we start a new round.
> 
> This patch changes to only "flush" the compression threads if we have
> "synched" with the kvm migration bitmap.  Idea is good but as far as I
> can see:
> 
> - we already call flush_compressed_data() inside firnd_dirty_block if we
>    synchronize the bitmap. 

The one called in find_dirty_block is as followings:

             if (migrate_use_xbzrle()) {
                 /* If xbzrle is on, stop using the data compression at this
                  * point. In theory, xbzrle can do better than compression.
                  */
                 flush_compressed_data(rs);
             }

We will call it only if xbzrle is also enabled, at this case, we will
disable compression and xbzrle for the following pages, please refer
to save_page_use_compression(). So, it can not help us if we just enabled
compression separately.

> So, at least, we need to update
>    dirty_sync_count there.

I understand this is a optimization that reduces flush_compressed_data
under the if both xbzrle and compression are both enabled. However, we
can avoid it by using save_page_use_compression() to replace
migrate_use_compression().

Furthermore, in our work which will be pushed it out after this patchset
(https://marc.info/?l=kvm&m=152810616708459&w=2), we will made
flush_compressed_data() really light if there is nothing to be flushed.

So, how about just keep it at this patch and let's optimize it in our
next work? :)

> 
> - queued pages are "interesting", but I am not sure if compression and
>    postcopy work well together.
> 

Compression and postcopy can not both be enabled, please refer to the
code in migrate_caps_check()

     if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
         if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
             /* The decompression threads asynchronously write into RAM
              * rather than use the atomic copies needed to avoid
              * userfaulting.  It should be possible to fix the decompression
              * threads for compatibility in future.
              */
             error_setg(errp, "Postcopy is not currently compatible "
                        "with compression");
             return false;
         }

> So, if we don't need to call flush_compressed_data() every round, then
> the one inside find_dirty_block() should be enough.  Otherwise, I can't
> see why we need this other.
> 
> 
> Independent of this patch:
> - We always send data for every compression thread without testing if
> there is any there.
> 

Yes. That's because we will never doubly handle the page in the iteration. :)

> 
>> @@ -3212,7 +3225,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>           }
>>           i++;
>>       }
>> -    flush_compressed_data(rs);
>>       rcu_read_unlock();
>>   
>>       /*
> 
> Why is not enough just to remove this call to flush_compressed_data?

Consider this case, thanks Dave to point it out. :)

===============

   iteration one:
      thread 1: Save compressed page 'n'

   iteration two:
      thread 2: Save compressed page 'n'

What guarantees that the version of page 'n'
from thread 2 reaches the destination first without
this flush?
===============

If we just remove it, we can not get this guarantee. :)