[PATCH] migration/dirtyrate: use QEMU_CLOCK_HOST to report start-time

[PATCH] migration/dirtyrate: use QEMU_CLOCK_HOST to report start-time

[Andrei Gudkov via]

Currently query-dirty-rate uses QEMU_CLOCK_REALTIME as
the source for start-time field. This translates to
clock_gettime(CLOCK_MONOTONIC), i.e. number of seconds
since host boot. This is not very useful. The only
reasonable use case of start-time I can imagine is to
check whether previously completed measurements are
too old or not. But this makes sense only if start-time
is reported as host wall-clock time.

This patch replaces source of start-time from
QEMU_CLOCK_REALTIME to QEMU_CLOCK_HOST.

Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
---
 qapi/migration.json   |  4 ++--
 migration/dirtyrate.c | 15 ++++++---------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 8843e74b59..63deb8e387 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1941,12 +1941,12 @@
 # 1. Measurement is in progress:
 #
 # <- {"status": "measuring", "sample-pages": 512,
-#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
+#     "mode": "page-sampling", "start-time": 1693900454, "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}
+#     "mode": "page-sampling", "start-time": 1693900454, "calc-time": 10}
 ##
 { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
 
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index bccb3515e3..0510d68765 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -259,11 +259,10 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
     return info;
 }
 
-static void init_dirtyrate_stat(int64_t start_time,
-                                struct DirtyRateConfig config)
+static void init_dirtyrate_stat(struct DirtyRateConfig config)
 {
     DirtyStat.dirty_rate = -1;
-    DirtyStat.start_time = start_time;
+    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
     DirtyStat.calc_time = config.sample_period_seconds;
     DirtyStat.sample_pages = config.sample_pages_per_gigabytes;
 
@@ -600,7 +599,7 @@ static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
     record_dirtypages_bitmap(&dirty_pages, true);
 
     start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-    DirtyStat.start_time = start_time / 1000;
+    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
 
     msec = config.sample_period_seconds * 1000;
     msec = dirty_stat_wait(msec, start_time);
@@ -628,7 +627,7 @@ static void calculate_dirtyrate_dirty_ring(struct DirtyRateConfig config)
     /* start log sync */
     global_dirty_log_change(GLOBAL_DIRTY_DIRTY_RATE, true);
 
-    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
+    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
 
     /* calculate vcpu dirtyrate */
     duration = vcpu_calculate_dirtyrate(config.sample_period_seconds * 1000,
@@ -657,6 +656,7 @@ static void calculate_dirtyrate_sample_vm(struct DirtyRateConfig config)
 
     rcu_read_lock();
     initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
     if (!record_ramblock_hash_info(&block_dinfo, config, &block_count)) {
         goto out;
     }
@@ -664,7 +664,6 @@ static void calculate_dirtyrate_sample_vm(struct DirtyRateConfig config)
 
     msec = config.sample_period_seconds * 1000;
     msec = dirty_stat_wait(msec, initial_time);
-    DirtyStat.start_time = initial_time / 1000;
     DirtyStat.calc_time = msec / 1000;
 
     rcu_read_lock();
@@ -727,7 +726,6 @@ void qmp_calc_dirty_rate(int64_t calc_time,
     static struct DirtyRateConfig config;
     QemuThread thread;
     int ret;
-    int64_t start_time;
 
     /*
      * If the dirty rate is already being measured, don't attempt to start.
@@ -799,8 +797,7 @@ void qmp_calc_dirty_rate(int64_t calc_time,
      **/
     dirtyrate_mode = mode;
 
-    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
-    init_dirtyrate_stat(start_time, config);
+    init_dirtyrate_stat(config);
 
     qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
                        (void *)&config, QEMU_THREAD_DETACHED);
-- 
2.30.2

Re: [PATCH] migration/dirtyrate: use QEMU_CLOCK_HOST to report start-time

[Philippe Mathieu-Daudé]

Hi Andrei,

On 5/9/23 11:18, Andrei Gudkov via wrote:
> Currently query-dirty-rate uses QEMU_CLOCK_REALTIME as
> the source for start-time field. This translates to
> clock_gettime(CLOCK_MONOTONIC), i.e. number of seconds
> since host boot. This is not very useful. The only
> reasonable use case of start-time I can imagine is to
> check whether previously completed measurements are
> too old or not. But this makes sense only if start-time
> is reported as host wall-clock time.
> 
> This patch replaces source of start-time from
> QEMU_CLOCK_REALTIME to QEMU_CLOCK_HOST.
> 
> Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> ---
>   qapi/migration.json   |  4 ++--
>   migration/dirtyrate.c | 15 ++++++---------
>   2 files changed, 8 insertions(+), 11 deletions(-)


> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index bccb3515e3..0510d68765 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -259,11 +259,10 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>       return info;
>   }
>   
> -static void init_dirtyrate_stat(int64_t start_time,
> -                                struct DirtyRateConfig config)
> +static void init_dirtyrate_stat(struct DirtyRateConfig config)
>   {
>       DirtyStat.dirty_rate = -1;
> -    DirtyStat.start_time = start_time;
> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
>       DirtyStat.calc_time = config.sample_period_seconds;
>       DirtyStat.sample_pages = config.sample_pages_per_gigabytes;
>   
> @@ -600,7 +599,7 @@ static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
>       record_dirtypages_bitmap(&dirty_pages, true);
>   
>       start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> -    DirtyStat.start_time = start_time / 1000;
> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;

You can directly use qemu_clock_get_us().

Re: [PATCH] migration/dirtyrate: use QEMU_CLOCK_HOST to report start-time

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Andrei,
>
> On 5/9/23 11:18, Andrei Gudkov via wrote:
>> Currently query-dirty-rate uses QEMU_CLOCK_REALTIME as
>> the source for start-time field. This translates to
>> clock_gettime(CLOCK_MONOTONIC), i.e. number of seconds
>> since host boot. This is not very useful. The only
>> reasonable use case of start-time I can imagine is to
>> check whether previously completed measurements are
>> too old or not. But this makes sense only if start-time
>> is reported as host wall-clock time.
>> This patch replaces source of start-time from
>> QEMU_CLOCK_REALTIME to QEMU_CLOCK_HOST.
>> Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
>> ---
>>   qapi/migration.json   |  4 ++--
>>   migration/dirtyrate.c | 15 ++++++---------
>>   2 files changed, 8 insertions(+), 11 deletions(-)
>
>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index bccb3515e3..0510d68765 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -259,11 +259,10 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>>       return info;
>>   }
>>   -static void init_dirtyrate_stat(int64_t start_time,
>> -                                struct DirtyRateConfig config)
>> +static void init_dirtyrate_stat(struct DirtyRateConfig config)
>>   {
>>       DirtyStat.dirty_rate = -1;
>> -    DirtyStat.start_time = start_time;
>> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
>>       DirtyStat.calc_time = config.sample_period_seconds;
>>       DirtyStat.sample_pages = config.sample_pages_per_gigabytes;
>>   @@ -600,7 +599,7 @@ static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
>>       record_dirtypages_bitmap(&dirty_pages, true);
>>         start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> -    DirtyStat.start_time = start_time / 1000;
>> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
>
> You can directly use qemu_clock_get_us().

Andrei, care to respin?

Re: [PATCH] migration/dirtyrate: use QEMU_CLOCK_HOST to report start-time

[gudkov.andrei--- via]

On Tue, Sep 19, 2023 at 11:14:52AM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> > Hi Andrei,
> >
> > On 5/9/23 11:18, Andrei Gudkov via wrote:
> >> Currently query-dirty-rate uses QEMU_CLOCK_REALTIME as
> >> the source for start-time field. This translates to
> >> clock_gettime(CLOCK_MONOTONIC), i.e. number of seconds
> >> since host boot. This is not very useful. The only
> >> reasonable use case of start-time I can imagine is to
> >> check whether previously completed measurements are
> >> too old or not. But this makes sense only if start-time
> >> is reported as host wall-clock time.
> >> This patch replaces source of start-time from
> >> QEMU_CLOCK_REALTIME to QEMU_CLOCK_HOST.
> >> Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> >> ---
> >>   qapi/migration.json   |  4 ++--
> >>   migration/dirtyrate.c | 15 ++++++---------
> >>   2 files changed, 8 insertions(+), 11 deletions(-)
> >
> >
> >> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> >> index bccb3515e3..0510d68765 100644
> >> --- a/migration/dirtyrate.c
> >> +++ b/migration/dirtyrate.c
> >> @@ -259,11 +259,10 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
> >>       return info;
> >>   }
> >>   -static void init_dirtyrate_stat(int64_t start_time,
> >> -                                struct DirtyRateConfig config)
> >> +static void init_dirtyrate_stat(struct DirtyRateConfig config)
> >>   {
> >>       DirtyStat.dirty_rate = -1;
> >> -    DirtyStat.start_time = start_time;
> >> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
> >>       DirtyStat.calc_time = config.sample_period_seconds;
> >>       DirtyStat.sample_pages = config.sample_pages_per_gigabytes;
> >>   @@ -600,7 +599,7 @@ static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> >>       record_dirtypages_bitmap(&dirty_pages, true);
> >>         start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >> -    DirtyStat.start_time = start_time / 1000;
> >> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
> >
> > You can directly use qemu_clock_get_us().
> 
> Andrei, care to respin?

But why? Here we need seconds, not microseconds. If there were a function
called qemu_clock_get_seconds(QEMUClockType) then we could use it here directly.
But there is no such function.

If you wish, we can do one of the following:
1) introduce qemu_clock_get_seconds(QEMUClockType) and use it directly
   without scaling
2) change the unit of DirtyStat.start_time from seconds to milliseconds
   and use qemu_clock_get_ms(QEMUClockType) directly without scaling

Re: [PATCH] migration/dirtyrate: use QEMU_CLOCK_HOST to report start-time

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 4784 bytes --]

On Tue, Sep 5, 2023 at 5:19 PM Andrei Gudkov <gudkov.andrei@huawei.com>
wrote:

> Currently query-dirty-rate uses QEMU_CLOCK_REALTIME as
> the source for start-time field. This translates to
> clock_gettime(CLOCK_MONOTONIC), i.e. number of seconds
> since host boot. This is not very useful. The only
> reasonable use case of start-time I can imagine is to
> check whether previously completed measurements are
> too old or not. But this makes sense only if start-time
> is reported as host wall-clock time.
>
> This patch replaces source of start-time from
> QEMU_CLOCK_REALTIME to QEMU_CLOCK_HOST.
>
> Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
> ---
>  qapi/migration.json   |  4 ++--
>  migration/dirtyrate.c | 15 ++++++---------
>  2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 8843e74b59..63deb8e387 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1941,12 +1941,12 @@
>  # 1. Measurement is in progress:
>  #
>  # <- {"status": "measuring", "sample-pages": 512,
> -#     "mode": "page-sampling", "start-time": 3665220, "calc-time": 10}
> +#     "mode": "page-sampling", "start-time": 1693900454, "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}
> +#     "mode": "page-sampling", "start-time": 1693900454, "calc-time": 10}
>  ##
>  { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index bccb3515e3..0510d68765 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -259,11 +259,10 @@ static struct DirtyRateInfo
> *query_dirty_rate_info(void)
>      return info;
>  }
>
> -static void init_dirtyrate_stat(int64_t start_time,
> -                                struct DirtyRateConfig config)
> +static void init_dirtyrate_stat(struct DirtyRateConfig config)
>  {
>      DirtyStat.dirty_rate = -1;
> -    DirtyStat.start_time = start_time;
> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
>      DirtyStat.calc_time = config.sample_period_seconds;
>      DirtyStat.sample_pages = config.sample_pages_per_gigabytes;
>
> @@ -600,7 +599,7 @@ static void calculate_dirtyrate_dirty_bitmap(struct
> DirtyRateConfig config)
>      record_dirtypages_bitmap(&dirty_pages, true);
>
>      start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> -    DirtyStat.start_time = start_time / 1000;
> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
>
>      msec = config.sample_period_seconds * 1000;
>      msec = dirty_stat_wait(msec, start_time);
> @@ -628,7 +627,7 @@ static void calculate_dirtyrate_dirty_ring(struct
> DirtyRateConfig config)
>      /* start log sync */
>      global_dirty_log_change(GLOBAL_DIRTY_DIRTY_RATE, true);
>
> -    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
>
>      /* calculate vcpu dirtyrate */
>      duration = vcpu_calculate_dirtyrate(config.sample_period_seconds *
> 1000,
> @@ -657,6 +656,7 @@ static void calculate_dirtyrate_sample_vm(struct
> DirtyRateConfig config)
>
>      rcu_read_lock();
>      initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    DirtyStat.start_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
>      if (!record_ramblock_hash_info(&block_dinfo, config, &block_count)) {
>          goto out;
>      }
> @@ -664,7 +664,6 @@ static void calculate_dirtyrate_sample_vm(struct
> DirtyRateConfig config)
>
>      msec = config.sample_period_seconds * 1000;
>      msec = dirty_stat_wait(msec, initial_time);
> -    DirtyStat.start_time = initial_time / 1000;
>      DirtyStat.calc_time = msec / 1000;
>
>      rcu_read_lock();
> @@ -727,7 +726,6 @@ void qmp_calc_dirty_rate(int64_t calc_time,
>      static struct DirtyRateConfig config;
>      QemuThread thread;
>      int ret;
> -    int64_t start_time;
>
>      /*
>       * If the dirty rate is already being measured, don't attempt to
> start.
> @@ -799,8 +797,7 @@ void qmp_calc_dirty_rate(int64_t calc_time,
>       **/
>      dirtyrate_mode = mode;
>
> -    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
> -    init_dirtyrate_stat(start_time, config);
> +    init_dirtyrate_stat(config);
>
>      qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
>                         (void *)&config, QEMU_THREAD_DETACHED);
> --
> 2.30.2
>
>

Reviewed-by: Hyman Huang <yong.huang@smartx.com>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 6521 bytes --]