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

[Li, Liang Z]

> 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,
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.

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.
>