Re: [Qemu-devel] [v5 02/12] migration: Add the framework of multi-thread compression

[Juan Quintela]

"Li, Liang Z" <address@hidden> wrote:
>> Reviewing patch 8, I found that we need to fix some things here.
>> 
>> > +static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
>> > +                                    ram_addr_t offset, bool
>> > +last_stage) {
>> > +    int bytes_sent = -1;
>> > +
>> > +    /* To be done*/
>> > +
>> > +    return bytes_sent;
>> > +}
>> 
>> We have three return values, here, that are not the same that for normal
>> pages
>> 
>>  0: this is the 1st page for a particular thread, nothing to sent yet  n > 
>> 0: we
>> are sending the previous compresed page for the choosen
>>         thread
>> 
>> Notice that the only way that ram_save_page() can return 0 is for xbzrle
>> when a page has modified but it has exactly the same value that before.
>> 
>> (it can have been modified twice, +1, -1 or whatever)
>> 
>> Notice that ram_save_page() can only return 0 (duplicate page) or > 0 (real
>> size written)
>> 
>> > +
>> >  /*
>> >   * ram_find_and_save_block: Finds a page to send and sends it to f
>> >   *
>> > @@ -679,7 +751,12 @@ static int ram_find_and_save_block(QEMUFile *f,
>> bool last_stage)
>> >                  ram_bulk_stage = false;
>> >              }
>> >          } else {
>> > -            bytes_sent = ram_save_page(f, block, offset, last_stage);
>> > +            if (migrate_use_compression()) {
>> > +                bytes_sent = ram_save_compressed_page(f, block, offset,
>> > +                                                      last_stage);
>> > +            } else {
>> > +                bytes_sent = ram_save_page(f, block, offset, last_stage);
>> > +            }
>> 
>> 
>> I need more context, this is the corrent code
>> 
>>         } else {
>>             bytes_sent = ram_save_page(f, block, offset, last_stage);
>> 
>>             /* if page is unmodified, continue to the next */
>>             if (bytes_sent > 0) {
>>                 last_sent_block = block;
>>                 break;
>>             }
>>         }
>> 
>> And we should change to:
>> 
>>         } else if (migrate_use_compression()) {
>>             bytes_sent = ram_save_compressed_page(f, block, offset,
>>                                                   last_stage);
>>             last_sent_block = block;
>>             break;
>
>
> What happened if ram_save_compressed_page() return 0 ?  following the
> flush_compressed_data() will be call,

This happens to me to send suggestions instead of proper code.
You are right.

I fixed one of the callers, but not the "upstream" caller.


> The code call still work, but the efficiency is poor. Every time the
> main thread find there is no free compression
> thread, it has to wait all compression threads finish their job before
> it can start the next round.
> The effective way is to start compression once there is any free
> compression thread.


Will send to the list a suggestion to improve from here, right?

Sorry for the noise, Juan.

> Liang
>
>>         } else {
>>             bytes_sent = ram_save_page(f, block, offset, last_stage);
>> 
>>             /* if page is unmodified, continue to the next */
>>             if (bytes_sent > 0) {
>>                 last_sent_block = block;
>>                 break;
>>             }
>>         }
>> 
>> This would mean that we don't need to arrange for the zero byte return on
>> qemu.
>>