Re: [PATCH v2 2/4] migration/calc-dirty-rate: detailed stats in sampling mode

[Markus Armbruster <armbru@redhat.com>]

Andrei Gudkov via <qemu-devel@nongnu.org> writes:

> Collect number of dirty pages for progresseively increasing time
> periods starting with 125ms up to number of seconds specified with
> calc-dirty-rate. Report through qmp and hmp: 1) vector of dirty page
> measurements, 2) page size, 3) total number of VM pages, 4) number
> of sampled pages.
>
> Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>

[...]

> diff --git a/qapi/migration.json b/qapi/migration.json
> index 2c35b7b9cf..f818f51e0e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1805,6 +1805,22 @@
   ##
   # @DirtyRateInfo:
   #
   # Information about current dirty page rate of vm.
   #
   # @dirty-rate: an estimate of the dirty page rate of the VM in units
   #     of MB/s, present only when estimating the rate has completed.
   #
   # @status: status containing dirtyrate query status includes
   #     'unstarted' or 'measuring' or 'measured'

Not this patch's fault, but here goes anyway:

0. "dirtyrate" isn't a word.  Spell it "dirty rate".  Many more
   instances elsewhere.

1. "status containing status"... what has the poor English language done
   to us that we torture it so?

2. "includes 'unstarted' or 'measuring' or 'measured' is confusing and
   entirely redundant with the type.  @status doesn't "include" these,
   these are the possible values, and all of them.

Suggest:

     @status: dirty rate measuring status

I do understand how difficult writing good English is for non-native
speakers.  This is mainly a failure of patch review.

   #
   # @start-time: start time in units of second for calculation
   #
   # @calc-time: time in units of second for sample dirty pages
   #
   # @sample-pages: page count per GB for sample dirty pages the default
   #     value is 512 (since 6.1)
   #
   # @mode: mode containing method of calculate dirtyrate includes
   #     'page-sampling' and 'dirty-ring' (Since 6.2)

Still not this patch's fault:

1. "mode containing method": more torture :)

2. "includes 'page-sampling' and 'dirty-ring'" is confusing.

   When it was added in commit 0e21bf24608, it was confusing and
   redundant like the text for @status above.

   Then commit 826b8bc80cb added value 'dirty-bitmap' without updating
   the member doc here.

Suggest:

     @mode: dirty rate measuring mode

   #
   # @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring mode
   #     specified (Since 6.2)
   #
> +# @page-size: page size in bytes (since 8.1)
> +#
> +# @n-total-pages: [page-sampling] total number of VM pages (since 8.1)
> +#
> +# @n-sampled-pages: [page-sampling] number of sampled VM pages (since 8.1)
> +#
> +# @periods: [page-sampling] array of time periods expressed in milliseconds
> +#           for which dirty-sample measurements were collected (since 8.1)
> +#
> +# @n-dirty-pages: [page-sampling] number of pages among all sampled pages
> +#                 that were observed as changed during respective time period.
> +#                 i-th element of this array corresponds to the i-th element
> +#                 of the @periods array, i.e. @n-dirty-pages[i] is the number
> +#                 of dirtied pages during period of @periods[i] milliseconds
> +#                 after the initiation of calc-dirty-rate (since 8.1)
> +#

Changed doc comment conventions landed yesterday (merge commit
568992e3440).  Please format like this:

   # @page-size: page size in bytes (since 8.1)
   #
   # @n-total-pages: [page-sampling] total number of VM pages (since 8.1)
   #
   # @n-sampled-pages: [page-sampling] number of sampled VM pages (since
   #     8.1)
   #
   # @n-zero-pages: [page-sampling] number of observed all-zero pages
   #     among all sampled pages (since 8.1)
   #
   # @periods: [page-sampling] array of time periods expressed in
   #     milliseconds for which dirty-sample measurements were collected
   #     (since 8.1)
   #
   # @n-dirty-pages: [page-sampling] number of pages among all sampled
   #     pages that were observed as changed during respective time
   #     period.  i-th element of this array corresponds to the i-th
   #     element of the @periods array, i.e. @n-dirty-pages[i] is the
   #     number of dirtied pages during period of @periods[i]
   #     milliseconds after the initiation of calc-dirty-rate (since 8.1)

The meaning of "[page-sampling]" is unclear.  What are you trying to
express?

For better or worse, we try to avoid abbreviations in QMP.  The "n-"
prefix is one.  What does it stand for?

It's quite unclear how all these numbers relate to each other.  What's
the difference between @n-sampled-pages and @sample-pages?  I think
we're missing an overview of the dirty rate measurement feature.

>  # Since: 5.2
>  ##
>  { 'struct': 'DirtyRateInfo',
> @@ -1814,7 +1830,13 @@
>             'calc-time': 'int64',
>             'sample-pages': 'uint64',
>             'mode': 'DirtyRateMeasureMode',
> -           '*vcpu-dirty-rate': [ 'DirtyRateVcpu' ] } }
> +           '*vcpu-dirty-rate': [ 'DirtyRateVcpu' ],
> +           'page-size': 'int64',

Shouldn't this be of type 'size'?

> +           '*n-total-pages': 'int64',
> +           '*n-sampled-pages': 'int64',
> +           '*periods': ['int64'],
> +           '*n-dirty-pages': ['int64'] } }

'uint64', like @sample-pages?

> +
>  
>  ##
>  # @calc-dirty-rate: