"Li, Liang Z" <liang.z...@intel.com> 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.
>>
