[PATCH] qapi: better docs for calc-dirty-rate and friends

[PATCH] qapi: better docs for calc-dirty-rate and friends

[Andrei Gudkov via]

Rewrote calc-dirty-rate documentation. Briefly described
different modes of dirty page rate measurement. Added some
examples. Fixed obvious grammar errors.

Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
---
 qapi/migration.json | 107 +++++++++++++++++++++++++++++++-------------
 1 file changed, 77 insertions(+), 30 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 179af0c4d8..19b51444b5 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1735,14 +1735,14 @@
 ##
 # @DirtyRateStatus:
 #
-# An enumeration of dirtyrate status.
+# Dirty page rate measurement status.
 #
-# @unstarted: the dirtyrate thread has not been started.
+# @unstarted: measuring thread has not been started yet
 #
-# @measuring: the dirtyrate thread is measuring.
+# @measuring: measuring thread is running
 #
-# @measured: the dirtyrate thread has measured and results are
-#     available.
+# @measured: dirty page rate is measured and the results are
+#     available
 #
 # Since: 5.2
 ##
@@ -1752,13 +1752,14 @@
 ##
 # @DirtyRateMeasureMode:
 #
-# An enumeration of mode of measuring dirtyrate.
+# Method used to measure dirty page rate.  Differences between
+# available methods are explained in @calc-dirty-rate.
 #
-# @page-sampling: calculate dirtyrate by sampling pages.
+# @page-sampling: use page sampling
 #
-# @dirty-ring: calculate dirtyrate by dirty ring.
+# @dirty-ring: use dirty ring
 #
-# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
+# @dirty-bitmap: use dirty bitmap
 #
 # Since: 6.2
 ##
@@ -1768,26 +1769,25 @@
 ##
 # @DirtyRateInfo:
 #
-# Information about current dirty page rate of vm.
+# Information about measured dirty page rate.
 #
 # @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.
+#     of MiB/s. Value is present only when @status is 'measured'.
 #
-# @status: status containing dirtyrate query status includes
-#     'unstarted' or 'measuring' or 'measured'
+# @status: current status of dirty page rate measurements
 #
 # @start-time: start time in units of second for calculation
 #
-# @calc-time: time in units of second for sample dirty pages
+# @calc-time: time period in units of second for which dirty page
+#     rate was measured
 #
-# @sample-pages: page count per GB for sample dirty pages the default
-#     value is 512 (since 6.1)
+# @sample-pages: number of sampled pages per each GiB of guest
+#     memory.  Value is valid only in page-sampling mode (Since 6.1)
 #
-# @mode: mode containing method of calculate dirtyrate includes
-#     'page-sampling' and 'dirty-ring' (Since 6.2)
+# @mode: mode that was used to measure dirty page rate (Since 6.2)
 #
-# @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring mode
-#     specified (Since 6.2)
+# @vcpu-dirty-rate: dirty rate for each vCPU if dirty-ring mode
+#     was specified (Since 6.2)
 #
 # Since: 5.2
 ##
@@ -1803,15 +1803,50 @@
 ##
 # @calc-dirty-rate:
 #
-# start calculating dirty page rate for vm
-#
-# @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: mechanism of calculating dirtyrate includes 'page-sampling'
-#     and 'dirty-ring' (Since 6.1)
+# Starts measuring dirty page rate of the VM.  Results can be
+# retrieved with @query-dirty-rate after measurements are completed.
+#
+# Dirty page rate is the number of pages changed in a given time
+# period expressed in MiB/s.  The following methods of calculation
+# are available:
+#
+# 1. In page sampling mode, a random subset of pages are selected
+#    and hashed twice: once in the beginning of measurement time
+#    period, another one -- in the end.  If two hashes for some page
+#    are different, the page is counted as changed.  Since this
+#    method relies on sampling and hashing, calculated dirty page
+#    rate is only the estimation of its true value.  Setting
+#    @sample-pages to higher value improves estimation quality but
+#    at the cost of higher computational overhead.
+#
+# 2. Dirty bitmap mode captures writes to memory by temporarily
+#    revoking write access to all pages and counting page faults.
+#    Information about modified pages is collected into bitmap,
+#    where each bit corresponds to one guest page.  This mode
+#    requires that KVM accelerator property "dirty-ring-size=N"
+#    is *not* set.
+#
+# 3. Dirty ring mode is similar to dirty bitmap mode, but the
+#    information about modified pages is collected into ring buffer.
+#    This mode tracks page modification per each vCPU separately.
+#    It requires that KVM accelerator property "dirty-ring-size=N"
+#    is set.
+#
+# @calc-time: time period in units of second for which dirty page rate
+#    is calculated.  Note that larger @calc-time values will typically
+#    result in smaller dirty page rates because page dirtying is a
+#    one-time event.  Once some page is counted as dirty during
+#    @calc-time period, further writes to this page will not increase
+#    dirty page rate anymore.
+#
+# @sample-pages: number of sampled pages per each GiB of guest memory.
+#     Default value is 512.  For 4KiB guest pages this corresponds to
+#     sampling ratio of 0.2%.  This argument is used only in page
+#     sampling mode. (Since 6.1)
+#
+# @mode: mechanism for tracking dirty pages.  Default value is
+#    'page-sampling'.  Others are 'dirty-bitmap' and 'dirty-ring'.
+#    (Since 6.1)
 #
 # Since: 5.2
 #
@@ -1828,9 +1863,21 @@
 ##
 # @query-dirty-rate:
 #
-# query dirty page rate in units of MB/s for vm
+# Query results of the most recent invocation of @calc-dirty-rate.
 #
 # Since: 5.2
+#
+# Examples:
+#
+# 1. Measurement is in progress:
+#
+# <- {"status": "measuring", "sample-pages": 512,
+#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
+#
+# 2. Measurement has been completed:
+#
+# <- {"status": "measured", "sample-pages": 512, "dirty-rate": 108,
+#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
 ##
 { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
 
-- 
2.30.2

Re: [PATCH] qapi: better docs for calc-dirty-rate and friends

[Markus Armbruster]

Andrei Gudkov <gudkov.andrei@huawei.com> writes:

> Rewrote calc-dirty-rate documentation. Briefly described
> different modes of dirty page rate measurement. Added some
> examples. Fixed obvious grammar errors.
>
> Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> ---
>  qapi/migration.json | 107 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 77 insertions(+), 30 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 179af0c4d8..19b51444b5 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1735,14 +1735,14 @@
>  ##
>  # @DirtyRateStatus:
>  #
> -# An enumeration of dirtyrate status.
> +# Dirty page rate measurement status.
>  #
> -# @unstarted: the dirtyrate thread has not been started.
> +# @unstarted: measuring thread has not been started yet
>  #
> -# @measuring: the dirtyrate thread is measuring.
> +# @measuring: measuring thread is running
>  #
> -# @measured: the dirtyrate thread has measured and results are
> -#     available.
> +# @measured: dirty page rate is measured and the results are
> +#     available
>  #
>  # Since: 5.2
>  ##
> @@ -1752,13 +1752,14 @@
>  ##
>  # @DirtyRateMeasureMode:
>  #
> -# An enumeration of mode of measuring dirtyrate.
> +# Method used to measure dirty page rate.  Differences between
> +# available methods are explained in @calc-dirty-rate.
>  #
> -# @page-sampling: calculate dirtyrate by sampling pages.
> +# @page-sampling: use page sampling
>  #
> -# @dirty-ring: calculate dirtyrate by dirty ring.
> +# @dirty-ring: use dirty ring
>  #
> -# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
> +# @dirty-bitmap: use dirty bitmap
>  #
>  # Since: 6.2
>  ##
> @@ -1768,26 +1769,25 @@
>  ##
>  # @DirtyRateInfo:
>  #
> -# Information about current dirty page rate of vm.
> +# Information about measured dirty page rate.
>  #
>  # @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.
> +#     of MiB/s. Value is present only when @status is 'measured'.

For consistency, please put two spaces between setences.

>  #
> -# @status: status containing dirtyrate query status includes
> -#     'unstarted' or 'measuring' or 'measured'
> +# @status: current status of dirty page rate measurements
>  #
>  # @start-time: start time in units of second for calculation
>  #
> -# @calc-time: time in units of second for sample dirty pages
> +# @calc-time: time period in units of second for which dirty page
> +#     rate was measured

Maybe

   # @calc-time: time period for which dirty page rate was measured
   #     (in seconds)

>  #
> -# @sample-pages: page count per GB for sample dirty pages the default
> -#     value is 512 (since 6.1)
> +# @sample-pages: number of sampled pages per each GiB of guest

per GiB

> +#     memory.  Value is valid only in page-sampling mode (Since 6.1)

Suggest "Valid only in ..."

>  #
> -# @mode: mode containing method of calculate dirtyrate includes
> -#     'page-sampling' and 'dirty-ring' (Since 6.2)
> +# @mode: mode that was used to measure dirty page rate (Since 6.2)
>  #
> -# @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring mode
> -#     specified (Since 6.2)
> +# @vcpu-dirty-rate: dirty rate for each vCPU if dirty-ring mode
> +#     was specified (Since 6.2)
>  #
>  # Since: 5.2
>  ##
> @@ -1803,15 +1803,50 @@
>  ##
>  # @calc-dirty-rate:
>  #
> -# start calculating dirty page rate for vm
> -#
> -# @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: mechanism of calculating dirtyrate includes 'page-sampling'
> -#     and 'dirty-ring' (Since 6.1)
> +# Starts measuring dirty page rate of the VM.  Results can be

Imperative mood: "Start measuring ..."

> +# retrieved with @query-dirty-rate after measurements are completed.
> +#
> +# Dirty page rate is the number of pages changed in a given time
> +# period expressed in MiB/s.  The following methods of calculation
> +# are available:
> +#
> +# 1. In page sampling mode, a random subset of pages are selected
> +#    and hashed twice: once in the beginning of measurement time

Suggest "once at the beginning"

> +#    period, another one -- in the end.  If two hashes for some page

Suggest ", and once again at the end".

> +#    are different, the page is counted as changed.  Since this
> +#    method relies on sampling and hashing, calculated dirty page
> +#    rate is only the estimation of its true value.  Setting
> +#    @sample-pages to higher value improves estimation quality but

Suggest "Increasing @sample-pages improves estimation quality at the
cost ..."

> +#    at the cost of higher computational overhead.
> +#
> +# 2. Dirty bitmap mode captures writes to memory by temporarily
> +#    revoking write access to all pages and counting page faults.

Comma before "and".

> +#    Information about modified pages is collected into bitmap,

"into a bitmap"

> +#    where each bit corresponds to one guest page.  This mode
> +#    requires that KVM accelerator property "dirty-ring-size=N"

Suggest just "dirty-ring-size" (omit "=N").

> +#    is *not* set.
> +#
> +# 3. Dirty ring mode is similar to dirty bitmap mode, but the
> +#    information about modified pages is collected into ring buffer.
> +#    This mode tracks page modification per each vCPU separately.

Either "for each vCPU" or "per vCPU".

> +#    It requires that KVM accelerator property "dirty-ring-size=N"
> +#    is set.

Suggest just "dirty-ring-size" (omit "=N").

> +#
> +# @calc-time: time period in units of second for which dirty page rate
> +#    is calculated.  Note that larger @calc-time values will typically
> +#    result in smaller dirty page rates because page dirtying is a
> +#    one-time event.  Once some page is counted as dirty during
> +#    @calc-time period, further writes to this page will not increase
> +#    dirty page rate anymore.

Please indent one more, for consistency.

> +#
> +# @sample-pages: number of sampled pages per each GiB of guest memory.
> +#     Default value is 512.  For 4KiB guest pages this corresponds to
> +#     sampling ratio of 0.2%.  This argument is used only in page
> +#     sampling mode. (Since 6.1)

Two spaces between '.' and '(', please.

> +#
> +# @mode: mechanism for tracking dirty pages.  Default value is
> +#    'page-sampling'.  Others are 'dirty-bitmap' and 'dirty-ring'.
> +#    (Since 6.1)
>  #
>  # Since: 5.2
>  #
> @@ -1828,9 +1863,21 @@
>  ##
>  # @query-dirty-rate:
>  #
> -# query dirty page rate in units of MB/s for vm
> +# Query results of the most recent invocation of @calc-dirty-rate.
>  #
>  # Since: 5.2
> +#
> +# Examples:
> +#
> +# 1. Measurement is in progress:
> +#
> +# <- {"status": "measuring", "sample-pages": 512,
> +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> +#
> +# 2. Measurement has been completed:
> +#
> +# <- {"status": "measured", "sample-pages": 512, "dirty-rate": 108,
> +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
>  ##
>  { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }

This is *sooo* much better than before.  Thank you!

An R-by from a migration maintainer would be nice.

If you agree with my suggestions, I can apply them in my tree, saving
you a respin.  Let me know.

Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH] qapi: better docs for calc-dirty-rate and friends

[Peter Xu]

On Thu, May 25, 2023 at 03:08:35PM +0200, Markus Armbruster wrote:
> Andrei Gudkov <gudkov.andrei@huawei.com> writes:
> 
> > Rewrote calc-dirty-rate documentation. Briefly described
> > different modes of dirty page rate measurement. Added some
> > examples. Fixed obvious grammar errors.
> >
> > Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> > ---
> >  qapi/migration.json | 107 +++++++++++++++++++++++++++++++-------------
> >  1 file changed, 77 insertions(+), 30 deletions(-)
> >
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 179af0c4d8..19b51444b5 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1735,14 +1735,14 @@
> >  ##
> >  # @DirtyRateStatus:
> >  #
> > -# An enumeration of dirtyrate status.
> > +# Dirty page rate measurement status.
> >  #
> > -# @unstarted: the dirtyrate thread has not been started.
> > +# @unstarted: measuring thread has not been started yet
> >  #
> > -# @measuring: the dirtyrate thread is measuring.
> > +# @measuring: measuring thread is running
> >  #
> > -# @measured: the dirtyrate thread has measured and results are
> > -#     available.
> > +# @measured: dirty page rate is measured and the results are
> > +#     available
> >  #
> >  # Since: 5.2
> >  ##
> > @@ -1752,13 +1752,14 @@
> >  ##
> >  # @DirtyRateMeasureMode:
> >  #
> > -# An enumeration of mode of measuring dirtyrate.
> > +# Method used to measure dirty page rate.  Differences between
> > +# available methods are explained in @calc-dirty-rate.
> >  #
> > -# @page-sampling: calculate dirtyrate by sampling pages.
> > +# @page-sampling: use page sampling
> >  #
> > -# @dirty-ring: calculate dirtyrate by dirty ring.
> > +# @dirty-ring: use dirty ring
> >  #
> > -# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
> > +# @dirty-bitmap: use dirty bitmap
> >  #
> >  # Since: 6.2
> >  ##
> > @@ -1768,26 +1769,25 @@
> >  ##
> >  # @DirtyRateInfo:
> >  #
> > -# Information about current dirty page rate of vm.
> > +# Information about measured dirty page rate.
> >  #
> >  # @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.
> > +#     of MiB/s. Value is present only when @status is 'measured'.
> 
> For consistency, please put two spaces between setences.
> 
> >  #
> > -# @status: status containing dirtyrate query status includes
> > -#     'unstarted' or 'measuring' or 'measured'
> > +# @status: current status of dirty page rate measurements
> >  #
> >  # @start-time: start time in units of second for calculation
> >  #
> > -# @calc-time: time in units of second for sample dirty pages
> > +# @calc-time: time period in units of second for which dirty page
> > +#     rate was measured
> 
> Maybe
> 
>    # @calc-time: time period for which dirty page rate was measured
>    #     (in seconds)
> 
> >  #
> > -# @sample-pages: page count per GB for sample dirty pages the default
> > -#     value is 512 (since 6.1)
> > +# @sample-pages: number of sampled pages per each GiB of guest
> 
> per GiB
> 
> > +#     memory.  Value is valid only in page-sampling mode (Since 6.1)
> 
> Suggest "Valid only in ..."
> 
> >  #
> > -# @mode: mode containing method of calculate dirtyrate includes
> > -#     'page-sampling' and 'dirty-ring' (Since 6.2)
> > +# @mode: mode that was used to measure dirty page rate (Since 6.2)
> >  #
> > -# @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring mode
> > -#     specified (Since 6.2)
> > +# @vcpu-dirty-rate: dirty rate for each vCPU if dirty-ring mode
> > +#     was specified (Since 6.2)
> >  #
> >  # Since: 5.2
> >  ##
> > @@ -1803,15 +1803,50 @@
> >  ##
> >  # @calc-dirty-rate:
> >  #
> > -# start calculating dirty page rate for vm
> > -#
> > -# @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: mechanism of calculating dirtyrate includes 'page-sampling'
> > -#     and 'dirty-ring' (Since 6.1)
> > +# Starts measuring dirty page rate of the VM.  Results can be
> 
> Imperative mood: "Start measuring ..."
> 
> > +# retrieved with @query-dirty-rate after measurements are completed.
> > +#
> > +# Dirty page rate is the number of pages changed in a given time
> > +# period expressed in MiB/s.  The following methods of calculation
> > +# are available:
> > +#
> > +# 1. In page sampling mode, a random subset of pages are selected
> > +#    and hashed twice: once in the beginning of measurement time
> 
> Suggest "once at the beginning"
> 
> > +#    period, another one -- in the end.  If two hashes for some page
> 
> Suggest ", and once again at the end".
> 
> > +#    are different, the page is counted as changed.  Since this
> > +#    method relies on sampling and hashing, calculated dirty page
> > +#    rate is only the estimation of its true value.  Setting
> > +#    @sample-pages to higher value improves estimation quality but
> 
> Suggest "Increasing @sample-pages improves estimation quality at the
> cost ..."
> 
> > +#    at the cost of higher computational overhead.
> > +#
> > +# 2. Dirty bitmap mode captures writes to memory by temporarily
> > +#    revoking write access to all pages and counting page faults.

Just to mention that wr-protection is only one way to do this, IIUC that
depends on both arch (e.g. s390 impl KVM_GET_DIRTY_LOG totally differently
from x86) and also hardware capabilities (e.g. even on x86, PML enabled
hosts use dirty bits and PML-full vm exits rather than page faults).

But I think wr-protect may still be a good detailed showcase of it so
keeping it there looks very nice, perhaps just add "... writes to memory,
for example, by temporarily revoking ..."?

> 
> Comma before "and".
> 
> > +#    Information about modified pages is collected into bitmap,
> 
> "into a bitmap"
> 
> > +#    where each bit corresponds to one guest page.  This mode
> > +#    requires that KVM accelerator property "dirty-ring-size=N"
> 
> Suggest just "dirty-ring-size" (omit "=N").
> 
> > +#    is *not* set.
> > +#
> > +# 3. Dirty ring mode is similar to dirty bitmap mode, but the
> > +#    information about modified pages is collected into ring buffer.
> > +#    This mode tracks page modification per each vCPU separately.
> 
> Either "for each vCPU" or "per vCPU".
> 
> > +#    It requires that KVM accelerator property "dirty-ring-size=N"
> > +#    is set.
> 
> Suggest just "dirty-ring-size" (omit "=N").
> 
> > +#
> > +# @calc-time: time period in units of second for which dirty page rate
> > +#    is calculated.  Note that larger @calc-time values will typically
> > +#    result in smaller dirty page rates because page dirtying is a
> > +#    one-time event.  Once some page is counted as dirty during
> > +#    @calc-time period, further writes to this page will not increase
> > +#    dirty page rate anymore.
> 
> Please indent one more, for consistency.
> 
> > +#
> > +# @sample-pages: number of sampled pages per each GiB of guest memory.
> > +#     Default value is 512.  For 4KiB guest pages this corresponds to
> > +#     sampling ratio of 0.2%.  This argument is used only in page
> > +#     sampling mode. (Since 6.1)
> 
> Two spaces between '.' and '(', please.
> 
> > +#
> > +# @mode: mechanism for tracking dirty pages.  Default value is
> > +#    'page-sampling'.  Others are 'dirty-bitmap' and 'dirty-ring'.
> > +#    (Since 6.1)
> >  #
> >  # Since: 5.2
> >  #
> > @@ -1828,9 +1863,21 @@
> >  ##
> >  # @query-dirty-rate:
> >  #
> > -# query dirty page rate in units of MB/s for vm
> > +# Query results of the most recent invocation of @calc-dirty-rate.
> >  #
> >  # Since: 5.2
> > +#
> > +# Examples:
> > +#
> > +# 1. Measurement is in progress:
> > +#
> > +# <- {"status": "measuring", "sample-pages": 512,
> > +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> > +#
> > +# 2. Measurement has been completed:
> > +#
> > +# <- {"status": "measured", "sample-pages": 512, "dirty-rate": 108,
> > +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> >  ##
> >  { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
> 
> This is *sooo* much better than before.  Thank you!

I also agree. :)

> 
> An R-by from a migration maintainer would be nice.

Only one migration maintainer now, but we have two reviewers.

Here's one from the reviewers' list:

Acked-by: Peter Xu <peterx@redhat.com>

> 
> If you agree with my suggestions, I can apply them in my tree, saving
> you a respin.  Let me know.
> 
> Acked-by: Markus Armbruster <armbru@redhat.com>
> 

-- 
Peter Xu

Re: [PATCH] qapi: better docs for calc-dirty-rate and friends

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 25, 2023 at 03:08:35PM +0200, Markus Armbruster wrote:
>> Andrei Gudkov <gudkov.andrei@huawei.com> writes:
>> 
>> > Rewrote calc-dirty-rate documentation. Briefly described
>> > different modes of dirty page rate measurement. Added some
>> > examples. Fixed obvious grammar errors.
>> >
>> > Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
>> > ---
>> >  qapi/migration.json | 107 +++++++++++++++++++++++++++++++-------------
>> >  1 file changed, 77 insertions(+), 30 deletions(-)
>> >
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index 179af0c4d8..19b51444b5 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json

[...]

>> > +# Dirty page rate is the number of pages changed in a given time
>> > +# period expressed in MiB/s.  The following methods of calculation
>> > +# are available:
>> > +#
>> > +# 1. In page sampling mode, a random subset of pages are selected
>> > +#    and hashed twice: once in the beginning of measurement time
>> 
>> Suggest "once at the beginning"
>> 
>> > +#    period, another one -- in the end.  If two hashes for some page
>> 
>> Suggest ", and once again at the end".
>> 
>> > +#    are different, the page is counted as changed.  Since this
>> > +#    method relies on sampling and hashing, calculated dirty page
>> > +#    rate is only the estimation of its true value.  Setting
>> > +#    @sample-pages to higher value improves estimation quality but
>> 
>> Suggest "Increasing @sample-pages improves estimation quality at the
>> cost ..."
>> 
>> > +#    at the cost of higher computational overhead.
>> > +#
>> > +# 2. Dirty bitmap mode captures writes to memory by temporarily
>> > +#    revoking write access to all pages and counting page faults.
>
> Just to mention that wr-protection is only one way to do this, IIUC that
> depends on both arch (e.g. s390 impl KVM_GET_DIRTY_LOG totally differently
> from x86) and also hardware capabilities (e.g. even on x86, PML enabled
> hosts use dirty bits and PML-full vm exits rather than page faults).

Good point.

> But I think wr-protect may still be a good detailed showcase of it so
> keeping it there looks very nice, perhaps just add "... writes to memory,
> for example, by temporarily revoking ..."?

Going with

      # 2. Dirty bitmap mode captures writes to memory (for example by
      #    temporarily revoking write access to all pages) and counting page
      #    faults.  Information about modified pages is collected into a
      #    bitmap, where each bit corresponds to one guest page.  This mode
      #    requires that KVM accelerator property "dirty-ring-size" is *not*
      #    set.

if you don't mind.

[...]

>> This is *sooo* much better than before.  Thank you!
>
> I also agree. :)
>
>> 
>> An R-by from a migration maintainer would be nice.
>
> Only one migration maintainer now, but we have two reviewers.
>
> Here's one from the reviewers' list:
>
> Acked-by: Peter Xu <peterx@redhat.com>

Thanks!

[...]

Re: [PATCH] qapi: better docs for calc-dirty-rate and friends

[Peter Xu]

On Fri, May 26, 2023 at 01:23:07PM +0200, Markus Armbruster wrote:
> Going with
> 
>       # 2. Dirty bitmap mode captures writes to memory (for example by
>       #    temporarily revoking write access to all pages) and counting page
>       #    faults.  Information about modified pages is collected into a
>       #    bitmap, where each bit corresponds to one guest page.  This mode
>       #    requires that KVM accelerator property "dirty-ring-size" is *not*
>       #    set.
> 
> if you don't mind.

Looks good here, thanks!

-- 
Peter Xu

Re: [PATCH] qapi: better docs for calc-dirty-rate and friends

[gudkov.andrei--- via]

On Thu, May 25, 2023 at 03:08:35PM +0200, Markus Armbruster wrote:
> Andrei Gudkov <gudkov.andrei@huawei.com> writes:
> 
> > Rewrote calc-dirty-rate documentation. Briefly described
> > different modes of dirty page rate measurement. Added some
> > examples. Fixed obvious grammar errors.
> >
> > Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> > ---
> >  qapi/migration.json | 107 +++++++++++++++++++++++++++++++-------------
> >  1 file changed, 77 insertions(+), 30 deletions(-)
> >
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 179af0c4d8..19b51444b5 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1735,14 +1735,14 @@
> >  ##
> >  # @DirtyRateStatus:
> >  #
> > -# An enumeration of dirtyrate status.
> > +# Dirty page rate measurement status.
> >  #
> > -# @unstarted: the dirtyrate thread has not been started.
> > +# @unstarted: measuring thread has not been started yet
> >  #
> > -# @measuring: the dirtyrate thread is measuring.
> > +# @measuring: measuring thread is running
> >  #
> > -# @measured: the dirtyrate thread has measured and results are
> > -#     available.
> > +# @measured: dirty page rate is measured and the results are
> > +#     available
> >  #
> >  # Since: 5.2
> >  ##
> > @@ -1752,13 +1752,14 @@
> >  ##
> >  # @DirtyRateMeasureMode:
> >  #
> > -# An enumeration of mode of measuring dirtyrate.
> > +# Method used to measure dirty page rate.  Differences between
> > +# available methods are explained in @calc-dirty-rate.
> >  #
> > -# @page-sampling: calculate dirtyrate by sampling pages.
> > +# @page-sampling: use page sampling
> >  #
> > -# @dirty-ring: calculate dirtyrate by dirty ring.
> > +# @dirty-ring: use dirty ring
> >  #
> > -# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
> > +# @dirty-bitmap: use dirty bitmap
> >  #
> >  # Since: 6.2
> >  ##
> > @@ -1768,26 +1769,25 @@
> >  ##
> >  # @DirtyRateInfo:
> >  #
> > -# Information about current dirty page rate of vm.
> > +# Information about measured dirty page rate.
> >  #
> >  # @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.
> > +#     of MiB/s. Value is present only when @status is 'measured'.
> 
> For consistency, please put two spaces between setences.
> 
> >  #
> > -# @status: status containing dirtyrate query status includes
> > -#     'unstarted' or 'measuring' or 'measured'
> > +# @status: current status of dirty page rate measurements
> >  #
> >  # @start-time: start time in units of second for calculation
> >  #
> > -# @calc-time: time in units of second for sample dirty pages
> > +# @calc-time: time period in units of second for which dirty page
> > +#     rate was measured
> 
> Maybe
> 
>    # @calc-time: time period for which dirty page rate was measured
>    #     (in seconds)
> 
> >  #
> > -# @sample-pages: page count per GB for sample dirty pages the default
> > -#     value is 512 (since 6.1)
> > +# @sample-pages: number of sampled pages per each GiB of guest
> 
> per GiB
> 
> > +#     memory.  Value is valid only in page-sampling mode (Since 6.1)
> 
> Suggest "Valid only in ..."
> 
> >  #
> > -# @mode: mode containing method of calculate dirtyrate includes
> > -#     'page-sampling' and 'dirty-ring' (Since 6.2)
> > +# @mode: mode that was used to measure dirty page rate (Since 6.2)
> >  #
> > -# @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring mode
> > -#     specified (Since 6.2)
> > +# @vcpu-dirty-rate: dirty rate for each vCPU if dirty-ring mode
> > +#     was specified (Since 6.2)
> >  #
> >  # Since: 5.2
> >  ##
> > @@ -1803,15 +1803,50 @@
> >  ##
> >  # @calc-dirty-rate:
> >  #
> > -# start calculating dirty page rate for vm
> > -#
> > -# @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: mechanism of calculating dirtyrate includes 'page-sampling'
> > -#     and 'dirty-ring' (Since 6.1)
> > +# Starts measuring dirty page rate of the VM.  Results can be
> 
> Imperative mood: "Start measuring ..."
> 
> > +# retrieved with @query-dirty-rate after measurements are completed.
> > +#
> > +# Dirty page rate is the number of pages changed in a given time
> > +# period expressed in MiB/s.  The following methods of calculation
> > +# are available:
> > +#
> > +# 1. In page sampling mode, a random subset of pages are selected
> > +#    and hashed twice: once in the beginning of measurement time
> 
> Suggest "once at the beginning"
> 
> > +#    period, another one -- in the end.  If two hashes for some page
> 
> Suggest ", and once again at the end".
> 
> > +#    are different, the page is counted as changed.  Since this
> > +#    method relies on sampling and hashing, calculated dirty page
> > +#    rate is only the estimation of its true value.  Setting
> > +#    @sample-pages to higher value improves estimation quality but
> 
> Suggest "Increasing @sample-pages improves estimation quality at the
> cost ..."
> 
> > +#    at the cost of higher computational overhead.
> > +#
> > +# 2. Dirty bitmap mode captures writes to memory by temporarily
> > +#    revoking write access to all pages and counting page faults.
> 
> Comma before "and".
> 
> > +#    Information about modified pages is collected into bitmap,
> 
> "into a bitmap"
> 
> > +#    where each bit corresponds to one guest page.  This mode
> > +#    requires that KVM accelerator property "dirty-ring-size=N"
> 
> Suggest just "dirty-ring-size" (omit "=N").
> 
> > +#    is *not* set.
> > +#
> > +# 3. Dirty ring mode is similar to dirty bitmap mode, but the
> > +#    information about modified pages is collected into ring buffer.
> > +#    This mode tracks page modification per each vCPU separately.
> 
> Either "for each vCPU" or "per vCPU".
> 
> > +#    It requires that KVM accelerator property "dirty-ring-size=N"
> > +#    is set.
> 
> Suggest just "dirty-ring-size" (omit "=N").
> 
> > +#
> > +# @calc-time: time period in units of second for which dirty page rate
> > +#    is calculated.  Note that larger @calc-time values will typically
> > +#    result in smaller dirty page rates because page dirtying is a
> > +#    one-time event.  Once some page is counted as dirty during
> > +#    @calc-time period, further writes to this page will not increase
> > +#    dirty page rate anymore.
> 
> Please indent one more, for consistency.
> 
> > +#
> > +# @sample-pages: number of sampled pages per each GiB of guest memory.
> > +#     Default value is 512.  For 4KiB guest pages this corresponds to
> > +#     sampling ratio of 0.2%.  This argument is used only in page
> > +#     sampling mode. (Since 6.1)
> 
> Two spaces between '.' and '(', please.
> 
> > +#
> > +# @mode: mechanism for tracking dirty pages.  Default value is
> > +#    'page-sampling'.  Others are 'dirty-bitmap' and 'dirty-ring'.
> > +#    (Since 6.1)
> >  #
> >  # Since: 5.2
> >  #
> > @@ -1828,9 +1863,21 @@
> >  ##
> >  # @query-dirty-rate:
> >  #
> > -# query dirty page rate in units of MB/s for vm
> > +# Query results of the most recent invocation of @calc-dirty-rate.
> >  #
> >  # Since: 5.2
> > +#
> > +# Examples:
> > +#
> > +# 1. Measurement is in progress:
> > +#
> > +# <- {"status": "measuring", "sample-pages": 512,
> > +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> > +#
> > +# 2. Measurement has been completed:
> > +#
> > +# <- {"status": "measured", "sample-pages": 512, "dirty-rate": 108,
> > +#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> >  ##
> >  { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
> 
> This is *sooo* much better than before.  Thank you!
> 
> An R-by from a migration maintainer would be nice.
> 
> If you agree with my suggestions, I can apply them in my tree, saving
> you a respin.  Let me know.

Yes, sure. Please include also suggestion about wr-protect from Peter. Thanks.

> 
> Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH] qapi: better docs for calc-dirty-rate and friends

[Markus Armbruster]

<gudkov.andrei@huawei.com> writes:

> On Thu, May 25, 2023 at 03:08:35PM +0200, Markus Armbruster wrote:
>> Andrei Gudkov <gudkov.andrei@huawei.com> writes:
>> 
>> > Rewrote calc-dirty-rate documentation. Briefly described
>> > different modes of dirty page rate measurement. Added some
>> > examples. Fixed obvious grammar errors.
>> >
>> > Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
>> > ---
>> >  qapi/migration.json | 107 +++++++++++++++++++++++++++++++-------------
>> >  1 file changed, 77 insertions(+), 30 deletions(-)
>> >
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index 179af0c4d8..19b51444b5 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json

[...]

>> > -# @sample-pages: page count per GB for sample dirty pages the default
>> > -#     value is 512 (since 6.1)
>> > +# @sample-pages: number of sampled pages per each GiB of guest
>> 
>> per GiB
>> 
>> > +#     memory.  Value is valid only in page-sampling mode (Since 6.1)
>> 
>> Suggest "Valid only in ..."

Outside this patch's scope, but here goes anway: I think we should have
made this member optional, and present only when it's actually valid.
Too late.

[...]

>> > +# Dirty page rate is the number of pages changed in a given time
>> > +# period expressed in MiB/s.  The following methods of calculation
>> > +# are available:
>> > +#
>> > +# 1. In page sampling mode, a random subset of pages are selected
>> > +#    and hashed twice: once in the beginning of measurement time
>> 
>> Suggest "once at the beginning"
>> 
>> > +#    period, another one -- in the end.  If two hashes for some page
>> 
>> Suggest ", and once again at the end".
>> 
>> > +#    are different, the page is counted as changed.  Since this
>> > +#    method relies on sampling and hashing, calculated dirty page
>> > +#    rate is only the estimation of its true value.  Setting

I'll change this to "is only an estimate of" if you don't mind.

[...]

>> This is *sooo* much better than before.  Thank you!
>> 
>> An R-by from a migration maintainer would be nice.

Got Peter's Acked-by now; all set.

>> If you agree with my suggestions, I can apply them in my tree, saving
>> you a respin.  Let me know.
>
> Yes, sure. Please include also suggestion about wr-protect from Peter. Thanks.

Done.

>> Acked-by: Markus Armbruster <armbru@redhat.com>

Queued.  Thanks!