Re: [Qemu-devel] [v5 09/12] migration: Make compression co-work with xbzrle

[Juan Quintela]

"Li, Liang Z" <address@hidden> wrote:
>> Liang Li <address@hidden> wrote:
>> > Now, multiple thread compression can co-work with xbzrle. when xbzrle
>> > is on, multiple thread compression will only work at the first round
>> > of RAM data sync.
>> >
>> > Signed-off-by: Liang Li <address@hidden>
>> > Signed-off-by: Yang Zhang <address@hidden>
>> > Reviewed-by: Dr.David Alan Gilbert <address@hidden>
>> 
>> Drop this patch and just give an error when trying to set xbzrle and
>> compression?  User have to pick one and only one, no second guess him/her?
>> 
>
> Live migration can benefit from compression co-work with xbzrle. You know, 
> xbzrle 
> transfer the raw RAM pages to destination in the ram bulk stage, and
> after that, it transfers
> the diff data.

I don't have numbers, so it is just hand-waving for my part.

> The ram bulk stage is where compression can do
> optimization,

If we do compression in the bulk page, xbzrle cache is going to be empty
after that, so we need to re-send the whole page anyways (at least the
1st time).

> and beside
> the ram bulk stage, xbzrle may do better than compression  in some
> situation.

With your patch, there is no way to select xbzrle for bulk stage, and
compression for the rest.  So this is is not an argument for this patch O:-)

> So
> compression and xbzrle are not in conflict but complementary.

Oh, there is no conflict between them, I fully agree there.  The problem
is when to use one or when to use the other.

My proposal:
- user devices if it wants to use xbzrle or compression.  It is
  completely clear what is going to happen.

- with this patch:
  ram bulk stage use compressión and from there it uses xbzrle: this
  needs at least to be documented on the man page and command line,
  otherwise the user don't know.

- perhaps it is even a good idea to change the code to do

    if (is_zero_range(..))
        send_it_as_one_byte()
    if (it_is_on_bzrle_cache())
        send as bzrle()
    if (it is not on xbzrle_cache() {
        put it on xbzrle_cache()
        send it compressed()
    }

And I am sure that there are even more posibilities.  The problem is
that to choose one or another we should:
- meassure what is better
- decide what to implement
- document how it works

As far as I can see, here we are doing the second item without having
doing the 1st (or at least I haven't seen the results), and clearly
without doing the third.

> I think it's a pity if we limit the use to select only one of them. If
> there is no strong reason,
> I don't agree to drop this patch.

Then you need to at least add documentation to explain why/what you are
doing.  If user selectcts xbzrle, it is clear what it does.  If user
selects compress, it is also clear.  If it selects both, it is not clear
(for looking at the documentation).

Later, Juan.