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

[Wei Wang]

On 06/04/2020 03:28 AM, Richard Henderson wrote:
> On Wed, 29 Apr 2020 at 18:54, Wei Wang <wei.w.wang@intel.com> wrote:
> > +        if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
> > +            xbzrle_counters.encoding_rate = 0;
> > +        } else if (!encoded_size) {
> > +            xbzrle_counters.encoding_rate = UINT64_MAX;
> > +        } else {
> > +            xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
> > +        }
> With clang 10, this produces
>
>    CC      aarch64-softmmu/migration/ram.o
> /home/rth/qemu/qemu/migration/ram.c:919:45: error: implicit conversion
> from 'unsigned long' to 'double' changes value from
> 18446744073709551615 to 18446744073709551616
> [-Werror,-Wimplicit-int-float-conversion]
>              xbzrle_counters.encoding_rate = UINT64_MAX;
>                                            ~ ^~~~~~~~~~
> /usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
> # define UINT64_MAX             (__UINT64_C(18446744073709551615))
>                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/include/stdint.h:107:25: note: expanded from macro '__UINT64_C'
> #  define __UINT64_C(c) c ## UL
>                          ^~~~~~~
> <scratch space>:36:1: note: expanded from here
> 18446744073709551615UL
> ^~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
>
> UINT64_MAX apprears both arbitrary and wrong.
>
> The only way I can imagine encoded_size == 0 is unencoded_size == 0,
> so 0 seems like the correct answer.  Moreover, it really seems like
> the first test sufficiently covers that possibility.

It is possible that encoded_size==0, but unencoded_size !=0. For example,
a page is written with the same data that it already has.


> In addition, the only user of this value is
>
> > +        monitor_printf(mon, "xbzrle encoding rate: %0.2f\n",
> > +                       info->xbzrle_cache->encoding_rate);
> which would be quite happy to print NaN even if the 0/0 computation
> were to run.  Though as I say above, I don't think that's reachable.

The encoding_rate is expected to reflect if the page is xbzrle encoding 
friendly.
The larger, the more friendly, so 0 might not be a good representation here.

Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue?

Best,
Wei