Re: [PATCH v2] migration/xbzrle: add encoding rate

[Wei Wang]

On 04/28/2020 10:51 PM, Dr. David Alan Gilbert wrote:
> * Wei Wang (address@hidden) wrote:
> > Users may need to check the xbzrle encoding rate to know if the guest
> > memory is xbzrle encoding-friendly, and dynamically turn off the
> > encoding if the encoding rate is low.
> >
> > Signed-off-by: Yi Sun <address@hidden>
> > Signed-off-by: Wei Wang <address@hidden>
> > ---
> >   migration/migration.c |  1 +
> >   migration/ram.c       | 38 ++++++++++++++++++++++++++++++++++++--
> >   monitor/hmp-cmds.c    |  2 ++
> >   qapi/migration.json   |  5 ++++-
> >   4 files changed, 43 insertions(+), 3 deletions(-)
> >
> > ChangeLog:
> > - include the 3 bytes (ENCODING_FLAG_XBZRLE flag and encoded_len) when
> >    calculating the encoding rate. Similar to the compress rate
> >    calculation, the 8 byte RAM_SAVE_FLAG_CONTINUE flag isn't included in
> >    the calculation.
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 187ac04..e404213 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -930,6 +930,7 @@ static void populate_ram_info(MigrationInfo *info, 
> > MigrationState *s)
> >           info->xbzrle_cache->pages = xbzrle_counters.pages;
> >           info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
> >           info->xbzrle_cache->cache_miss_rate = xbzrle_counters.cache_miss_rate;
> > +        info->xbzrle_cache->encoding_rate = xbzrle_counters.encoding_rate;
> >           info->xbzrle_cache->overflow = xbzrle_counters.overflow;
> >       }
> >   
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 04f13fe..f46ab96 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -327,6 +327,10 @@ struct RAMState {
> >       uint64_t num_dirty_pages_period;
> >       /* xbzrle misses since the beginning of the period */
> >       uint64_t xbzrle_cache_miss_prev;
> > +    /* Amount of xbzrle pages since the beginning of the period */
> > +    uint64_t xbzrle_pages_prev;
> > +    /* Amount of xbzrle encoded bytes since the beginning of the period */
> > +    uint64_t xbzrle_bytes_prev;
> >   
> >       /* compression statistics since the beginning of the period */
> >       /* amount of count that no free thread to compress data */
> > @@ -696,6 +700,18 @@ static int save_xbzrle_page(RAMState *rs, uint8_t 
> > **current_data,
> >           return -1;
> >       }
> >   
> > +    /*
> > +     * Reaching here means the page has hit the xbzrle cache, no matter what
> > +     * encoding result it is (normal encoding, overflow or skipping the page),
> > +     * count the page as encoded. This is used to caculate the encoding rate.
> > +     *
> > +     * Example: 2 pages (8KB) being encoded, first page encoding generates 2KB,
> > +     * 2nd page turns out to be skipped (i.e. no new bytes written to the
> > +     * page), the overall encoding rate will be 8KB / 2KB = 4, which has the
> > +     * skipped page included. In this way, the encoding rate can tell if the
> > +     * guest page is good for xbzrle encoding.
> > +     */
> > +    xbzrle_counters.pages++;
> Can you explain how that works with overflow?  Doesn't overflow return
> -1 here, not increment the bytes, so it looks like you've xbzrle'd a
> page, but the encoding rate hasn't incremented.

OK. How about adding below before returning -1 (for the overflow case):

...
xbzrle_counters.bytes += TARGET_PAGE_SIZE;
return -1;

Example: if we have 2 pages encoded as below:
4KB--> after normal encoding: 2KB
4KB--> after overflow: 4KB (will be sent as non-encoded page)
then the encoding rate is 8KB / 6KB = ~1.3
(if we skip the counting of the overflow case,
the encoding rate will be 4KB/ 2KB=2. Users may think that's
good to go with xbzrle, unless they do another calculation with
checking the overflow numbers themselves)

Best,
Wei