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

[Dr. David Alan Gilbert]

* Wei Wang (address@hidden) wrote:
> 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;

Yes, I think that feels better.

Dave

> 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
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK