Re: [PATCH v5 11/12] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Zheng Chuan]

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.
>