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

[gudkov.andrei]

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