Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 9f52f5f..08c46d3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -62,6 +62,28 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+static struct DirtyRateInfo *query_dirty_rate_info(void)
+{
+    int64_t dirty_rate = DirtyStat.dirty_rate;
+    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
+
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
+        info->dirty_rate = dirty_rate;
+    } else {
+        info->dirty_rate = -1;
+    }
+
+    info->status = CalculatingState;
+    /*
+     * Only support query once for each calculation,
+     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
+     */
+    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
+                              DIRTY_RATE_STATUS_UNSTARTED);
+
+    return info;
+}
+
 static void reset_dirtyrate_stat(void)
 {
     DirtyStat.total_dirty_samples = 0;
@@ -378,3 +400,26 @@ void *get_dirtyrate_thread(void *arg)
                               DIRTY_RATE_STATUS_MEASURED);
     return NULL;
 }
+
+void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
+{
+    static struct DirtyRateConfig config;
+    QemuThread thread;
+
+    /*
+     * We don't begin calculating thread only when it's in calculating status.
+     */
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
+        return;
+    }
+
+    config.sample_period_seconds = get_sample_page_period(calc_time);
+    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
+                       (void *)&config, QEMU_THREAD_DETACHED);
+}
+
+struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
+{
+    return query_dirty_rate_info();
+}
diff --git a/qapi/migration.json b/qapi/migration.json
index d640165..826bfd7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1737,3 +1737,47 @@
 ##
 { 'enum': 'DirtyRateStatus',
   'data': [ 'unstarted', 'measuring', 'measured'] }
+
+##
+# @DirtyRateInfo:
+#
+# Information about current dirty page rate of vm.
+#
+# @dirty-rate: @dirtyrate describing the dirty page rate of vm
+#          in units of MB/s.
+#          If this field return '-1', it means querying is not
+#          start or not complete.
+#
+# @status: status containing dirtyrate query status includes
+#          'unstarted' or 'measuring' or 'measured'
+#
+# Since: 5.2
+#
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'DirtyRateStatus'} }
+
+##
+# @calc-dirty-rate:
+#
+# start calculating dirty page rate for vm
+#
+# @calc-time: time in units of second for sample dirty pages
+#
+# Since: 5.2
+#
+# Example:
+#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }
+#
+##
+{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
+
+##
+# @query-dirty-rate:
+#
+# query dirty page rate in units of MB/s for vm
+#
+# Since: 5.2
+##
+{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
-- 
1.8.3.1

On Monday, 2020-08-24 at 17:14:39 +08, Chuan Zheng wrote:

> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 9f52f5f..08c46d3 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -62,6 +62,28 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>      }
>  }
>  
> +static struct DirtyRateInfo *query_dirty_rate_info(void)
> +{
> +    int64_t dirty_rate = DirtyStat.dirty_rate;
> +    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
> +
> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
> +        info->dirty_rate = dirty_rate;
> +    } else {
> +        info->dirty_rate = -1;
> +    }
> +
> +    info->status = CalculatingState;
> +    /*
> +     * Only support query once for each calculation,
> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
> +     */
> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
> +                              DIRTY_RATE_STATUS_UNSTARTED);

Is there a reason for this restriction? Removing it would require
clarifying the state model, I suppose.

> +
> +    return info;
> +}
> +
>  static void reset_dirtyrate_stat(void)
>  {
>      DirtyStat.total_dirty_samples = 0;
> @@ -378,3 +400,26 @@ void *get_dirtyrate_thread(void *arg)
>                                DIRTY_RATE_STATUS_MEASURED);
>      return NULL;
>  }
> +
> +void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
> +{
> +    static struct DirtyRateConfig config;
> +    QemuThread thread;
> +
> +    /*
> +     * We don't begin calculating thread only when it's in calculating status.

"If the dirty rate is already being measured, don't attempt to start."

> +     */
> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {

Should this return an error to the caller?

> +        return;
> +    }
> +
> +    config.sample_period_seconds = get_sample_page_period(calc_time);

If the specified sample period is outside the min/max, should an error
be returned to the caller?

> +    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> +    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
> +                       (void *)&config, QEMU_THREAD_DETACHED);
> +}
> +
> +struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
> +{
> +    return query_dirty_rate_info();
> +}
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d640165..826bfd7 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1737,3 +1737,47 @@
>  ##
>  { 'enum': 'DirtyRateStatus',
>    'data': [ 'unstarted', 'measuring', 'measured'] }
> +
> +##
> +# @DirtyRateInfo:
> +#
> +# Information about current dirty page rate of vm.
> +#
> +# @dirty-rate: @dirtyrate describing the dirty page rate of vm
> +#          in units of MB/s.
> +#          If this field return '-1', it means querying is not
> +#          start or not complete.
> +#
> +# @status: status containing dirtyrate query status includes
> +#          'unstarted' or 'measuring' or 'measured'
> +#
> +# Since: 5.2
> +#
> +##
> +{ 'struct': 'DirtyRateInfo',
> +  'data': {'dirty-rate': 'int64',
> +           'status': 'DirtyRateStatus'} }
> +
> +##
> +# @calc-dirty-rate:
> +#
> +# start calculating dirty page rate for vm
> +#
> +# @calc-time: time in units of second for sample dirty pages
> +#
> +# Since: 5.2
> +#
> +# Example:
> +#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }

"cal-dirty-rate" -> "calc-dirty-rate".

> +#
> +##
> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
> +
> +##
> +# @query-dirty-rate:
> +#
> +# query dirty page rate in units of MB/s for vm
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
> -- 
> 1.8.3.1

dme.
-- 
I'm going in the out door, I'm doing all right.

On 2020/8/26 18:26, David Edmondson wrote:
> On Monday, 2020-08-24 at 17:14:39 +08, Chuan Zheng wrote:
> 
>> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> ---
>>  migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 89 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 9f52f5f..08c46d3 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -62,6 +62,28 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>>      }
>>  }
>>  
>> +static struct DirtyRateInfo *query_dirty_rate_info(void)
>> +{
>> +    int64_t dirty_rate = DirtyStat.dirty_rate;
>> +    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
>> +
>> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
>> +        info->dirty_rate = dirty_rate;
>> +    } else {
>> +        info->dirty_rate = -1;
>> +    }
>> +
>> +    info->status = CalculatingState;
>> +    /*
>> +     * Only support query once for each calculation,
>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>> +     */
>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>> +                              DIRTY_RATE_STATUS_UNSTARTED);
> 
> Is there a reason for this restriction? Removing it would require
> clarifying the state model, I suppose.
> 
We only support query once for each calculation.
Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
long time ago.

>> +
>> +    return info;
>> +}
>> +
>>  static void reset_dirtyrate_stat(void)
>>  {
>>      DirtyStat.total_dirty_samples = 0;
>> @@ -378,3 +400,26 @@ void *get_dirtyrate_thread(void *arg)
>>                                DIRTY_RATE_STATUS_MEASURED);
>>      return NULL;
>>  }
>> +
>> +void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>> +{
>> +    static struct DirtyRateConfig config;
>> +    QemuThread thread;
>> +
>> +    /*
>> +     * We don't begin calculating thread only when it's in calculating status.
> 
> "If the dirty rate is already being measured, don't attempt to start."
> 
>> +     */
>> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
> 
> Should this return an error to the caller?
> 
>> +        return;
>> +    }
>> +
>> +    config.sample_period_seconds = get_sample_page_period(calc_time);
> 
> If the specified sample period is outside the min/max, should an error
> be returned to the caller?
> 
>> +    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>> +    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
>> +                       (void *)&config, QEMU_THREAD_DETACHED);
>> +}
>> +
>> +struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
>> +{
>> +    return query_dirty_rate_info();
>> +}
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d640165..826bfd7 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1737,3 +1737,47 @@
>>  ##
>>  { 'enum': 'DirtyRateStatus',
>>    'data': [ 'unstarted', 'measuring', 'measured'] }
>> +
>> +##
>> +# @DirtyRateInfo:
>> +#
>> +# Information about current dirty page rate of vm.
>> +#
>> +# @dirty-rate: @dirtyrate describing the dirty page rate of vm
>> +#          in units of MB/s.
>> +#          If this field return '-1', it means querying is not
>> +#          start or not complete.
>> +#
>> +# @status: status containing dirtyrate query status includes
>> +#          'unstarted' or 'measuring' or 'measured'
>> +#
>> +# Since: 5.2
>> +#
>> +##
>> +{ 'struct': 'DirtyRateInfo',
>> +  'data': {'dirty-rate': 'int64',
>> +           'status': 'DirtyRateStatus'} }
>> +
>> +##
>> +# @calc-dirty-rate:
>> +#
>> +# start calculating dirty page rate for vm
>> +#
>> +# @calc-time: time in units of second for sample dirty pages
>> +#
>> +# Since: 5.2
>> +#
>> +# Example:
>> +#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }
> 
> "cal-dirty-rate" -> "calc-dirty-rate".
> 
>> +#
>> +##
>> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
>> +
>> +##
>> +# @query-dirty-rate:
>> +#
>> +# query dirty page rate in units of MB/s for vm
>> +#
>> +# Since: 5.2
>> +##
>> +{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
>> -- 
>> 1.8.3.1
> 
> dme.
> 

On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:

>>> +    /*
>>> +     * Only support query once for each calculation,
>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>> +     */
>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>> 
>> Is there a reason for this restriction? Removing it would require
>> clarifying the state model, I suppose.
>> 
> We only support query once for each calculation.
> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
> long time ago.

There's nothing in the current interface that prevents this from being
the case already - the caller could initiate a 1 second sample, then
wait 24 hours to query the result.

Obviously this would generally be regarded as "d'oh - don't do that",
but the same argument would apply if the caller is allowed to query the
results multiple times.

Perhaps a complete solution would be to include information about the
sample period with the result. The caller could then determine whether
the sample is of adequate quality (sufficiently recent, taken over a
sufficiently long time period) for its' intended use.

dme.
-- 
I walk like a building, I never get wet.

On 2020/8/27 19:58, David Edmondson wrote:
> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
> 
>>>> +    /*
>>>> +     * Only support query once for each calculation,
>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>> +     */
>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>
>>> Is there a reason for this restriction? Removing it would require
>>> clarifying the state model, I suppose.
>>>
>> We only support query once for each calculation.
>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>> long time ago.
> 
> There's nothing in the current interface that prevents this from being
> the case already - the caller could initiate a 1 second sample, then
> wait 24 hours to query the result.
> 
> Obviously this would generally be regarded as "d'oh - don't do that",
> but the same argument would apply if the caller is allowed to query the
> results multiple times.
> 
> Perhaps a complete solution would be to include information about the
> sample period with the result. The caller could then determine whether
> the sample is of adequate quality (sufficiently recent, taken over a
> sufficiently long time period) for its' intended use.
> 
You mean add timestamp when i calculate?
Actually, I do not want make it complicate for qemu code,
maybe it could be left for user to implement both two qmp commands
like in libvirt-api.

On the other hand, it really bother me that we need to reset calculating state
to make sure the state model could be restart in next calculation.

For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(

Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().

> dme.
> 

On Thursday, 2020-08-27 at 20:55:51 +08, Zheng Chuan wrote:

> On 2020/8/27 19:58, David Edmondson wrote:
>> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
>> 
>>>>> +    /*
>>>>> +     * Only support query once for each calculation,
>>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>>> +     */
>>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>>
>>>> Is there a reason for this restriction? Removing it would require
>>>> clarifying the state model, I suppose.
>>>>
>>> We only support query once for each calculation.
>>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>>> long time ago.
>> 
>> There's nothing in the current interface that prevents this from being
>> the case already - the caller could initiate a 1 second sample, then
>> wait 24 hours to query the result.
>> 
>> Obviously this would generally be regarded as "d'oh - don't do that",
>> but the same argument would apply if the caller is allowed to query the
>> results multiple times.
>> 
>> Perhaps a complete solution would be to include information about the
>> sample period with the result. The caller could then determine whether
>> the sample is of adequate quality (sufficiently recent, taken over a
>> sufficiently long time period) for its' intended use.
>> 
> You mean add timestamp when i calculate?

You already have a timestamp, though I'm not sure if it is one that is
appropriate to report to a user.

I was thinking that you would include both the start time and duration
of the sample in the output of the query-dirty-rate QMP command, as well
as the dirty rate itself. That way the caller can make a decision about
whether the data is useful.

> Actually, I do not want make it complicate for qemu code,
> maybe it could be left for user to implement both two qmp commands
> like in libvirt-api.

Sorry, I didn't understand this comment.

> On the other hand, it really bother me that we need to reset calculating state
> to make sure the state model could be restart in next calculation.
>
> For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(
>
> Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().
>
>> dme.
>> 

dme.
-- 
Another lonely day, no one here but me-o.

On 2020/8/27 21:07, David Edmondson wrote:
> On Thursday, 2020-08-27 at 20:55:51 +08, Zheng Chuan wrote:
> 
>> On 2020/8/27 19:58, David Edmondson wrote:
>>> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
>>>
>>>>>> +    /*
>>>>>> +     * Only support query once for each calculation,
>>>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>>>> +     */
>>>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>>>
>>>>> Is there a reason for this restriction? Removing it would require
>>>>> clarifying the state model, I suppose.
>>>>>
>>>> We only support query once for each calculation.
>>>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>>>> long time ago.
>>>
>>> There's nothing in the current interface that prevents this from being
>>> the case already - the caller could initiate a 1 second sample, then
>>> wait 24 hours to query the result.
>>>
>>> Obviously this would generally be regarded as "d'oh - don't do that",
>>> but the same argument would apply if the caller is allowed to query the
>>> results multiple times.
>>>
>>> Perhaps a complete solution would be to include information about the
>>> sample period with the result. The caller could then determine whether
>>> the sample is of adequate quality (sufficiently recent, taken over a
>>> sufficiently long time period) for its' intended use.
>>>
>> You mean add timestamp when i calculate?
> 
> You already have a timestamp, though I'm not sure if it is one that is
> appropriate to report to a user.
> 
> I was thinking that you would include both the start time and duration
> of the sample in the output of the query-dirty-rate QMP command, as well
> as the dirty rate itself. That way the caller can make a decision about
> whether the data is useful.
> 
OK, i understand.
I may add it like this:
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'DirtyRateStatus',
+           'start-timestamp': 'int64',
+           'calc-time': 'int64'} }
+
+##
the stat-timestamp would be initial_time which gets from qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
at the beginning of calculation while calc_time is time-duration in microsecond.

But i reconsider that, it maybe still need to reset the CalculatingState as DIRTY_RATE_STATUS_UNSTARTED
here?

Initialization like:
void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
{
   XXXX

    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
        return;
    }


    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
                              DIRTY_RATE_STATUS_UNSTARTED);
    XXXX
}

It could not prevent concurrent scene which may lead to disorder state:(


>> Actually, I do not want make it complicate for qemu code,
>> maybe it could be left for user to implement both two qmp commands
>> like in libvirt-api.
> 
> Sorry, I didn't understand this comment.
> 
>> On the other hand, it really bother me that we need to reset calculating state
>> to make sure the state model could be restart in next calculation.
>>
>> For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(
>>
>> Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().
>>
>>> dme.
>>>
> 
> dme.
> 

On Thursday, 2020-08-27 at 22:47:15 +08, Zheng Chuan wrote:

> On 2020/8/27 21:07, David Edmondson wrote:
>> On Thursday, 2020-08-27 at 20:55:51 +08, Zheng Chuan wrote:
>> 
>>> On 2020/8/27 19:58, David Edmondson wrote:
>>>> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
>>>>
>>>>>>> +    /*
>>>>>>> +     * Only support query once for each calculation,
>>>>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>>>>> +     */
>>>>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>>>>
>>>>>> Is there a reason for this restriction? Removing it would require
>>>>>> clarifying the state model, I suppose.
>>>>>>
>>>>> We only support query once for each calculation.
>>>>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>>>>> long time ago.
>>>>
>>>> There's nothing in the current interface that prevents this from being
>>>> the case already - the caller could initiate a 1 second sample, then
>>>> wait 24 hours to query the result.
>>>>
>>>> Obviously this would generally be regarded as "d'oh - don't do that",
>>>> but the same argument would apply if the caller is allowed to query the
>>>> results multiple times.
>>>>
>>>> Perhaps a complete solution would be to include information about the
>>>> sample period with the result. The caller could then determine whether
>>>> the sample is of adequate quality (sufficiently recent, taken over a
>>>> sufficiently long time period) for its' intended use.
>>>>
>>> You mean add timestamp when i calculate?
>> 
>> You already have a timestamp, though I'm not sure if it is one that is
>> appropriate to report to a user.
>> 
>> I was thinking that you would include both the start time and duration
>> of the sample in the output of the query-dirty-rate QMP command, as well
>> as the dirty rate itself. That way the caller can make a decision about
>> whether the data is useful.
>> 
> OK, i understand.
> I may add it like this:
> +##
> +{ 'struct': 'DirtyRateInfo',
> +  'data': {'dirty-rate': 'int64',
> +           'status': 'DirtyRateStatus',
> +           'start-timestamp': 'int64',
> +           'calc-time': 'int64'} }
> +
> +##
> the stat-timestamp would be initial_time which gets from qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
> at the beginning of calculation while calc_time is time-duration in microsecond.

The calc-time reported here should be in the same units as when it is
specified in calc-dirty-rate (seconds for both seems fine).

I suspect that providing the start-timestamp in seconds would also be
fine - it's not obvious that knowing the value in milliseconds adds much
value.

> But i reconsider that, it maybe still need to reset the CalculatingState as DIRTY_RATE_STATUS_UNSTARTED
> here?
>
> Initialization like:
> void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
> {
>    XXXX
>
>     if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
>         return;
>     }
>
>
>     (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>                               DIRTY_RATE_STATUS_UNSTARTED);
>     XXXX
> }
>
> It could not prevent concurrent scene which may lead to disorder state:(

It should be possible to initiate measurement when the state is either
UNSTARTED or MEASURED - only MEASURING should rule it out.

>
>
>>> Actually, I do not want make it complicate for qemu code,
>>> maybe it could be left for user to implement both two qmp commands
>>> like in libvirt-api.
>> 
>> Sorry, I didn't understand this comment.
>> 
>>> On the other hand, it really bother me that we need to reset calculating state
>>> to make sure the state model could be restart in next calculation.
>>>
>>> For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(
>>>
>>> Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().
>>>
>>>> dme.
>>>>
>> 
>> dme.
>> 

dme.
-- 
Everyone I know, goes away in the end.