Re: [PATCH v2 3/8] migration: show the statistics of compression

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
> @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>              rs->xbzrle_cache_miss_prev) / iter_count;
>          rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>      }
> +
> +    if (migrate_use_compression()) {
> +        uint64_t comp_pages;
> +
> +        compression_counters.busy_rate = (double)(compression_counters.busy -
> +            rs->compress_thread_busy_prev) / iter_count;

Here I'm not sure it's correct...

"iter_count" stands for ramstate.iterations.  It's increased per
ram_find_and_save_block(), so IMHO it might contain multiple guest
pages.  However compression_counters.busy should be per guest page.

> +        rs->compress_thread_busy_prev = compression_counters.busy;
> +
> +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
> +        if (comp_pages) {
> +            compression_counters.compression_rate =
> +                (double)(compression_counters.reduced_size -
> +                rs->compress_reduced_size_prev) /
> +                (comp_pages * TARGET_PAGE_SIZE);
> +            rs->compress_pages_prev = compression_counters.pages;
> +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
> +        }
> +    }
>  }
>  
>  static void migration_bitmap_sync(RAMState *rs)
> @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
>          qemu_mutex_lock(&comp_param[idx].mutex);
>          if (!comp_param[idx].quit) {
>              len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;

I would agree with Dave here - why we store the "reduced size" instead
of the size of the compressed data (which I think should be len - 8)?

Meanwhile, would a helper be nicer? Like:

        void migrate_compressed_page_stats_update(int xfer_size)
        {
                /* Removing the offset header in save_page_header() */
                compression_counters.compressed_size = xfer_size - 8;
                compression_counters.pages++;
                ram_counters.transferred += bytes_xmit;
        }

> +            compression_counters.pages++;
>              ram_counters.transferred += len;
>          }
>          qemu_mutex_unlock(&comp_param[idx].mutex);
> @@ -1903,6 +1935,10 @@ retry:
>              qemu_cond_signal(&comp_param[idx].cond);
>              qemu_mutex_unlock(&comp_param[idx].mutex);
>              pages = 1;
> +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> +            compression_counters.reduced_size += TARGET_PAGE_SIZE -
> +                                                 bytes_xmit + 8;
> +            compression_counters.pages++;
>              ram_counters.transferred += bytes_xmit;
>              break;
>          }

Regards,

-- 
Peter Xu