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

[David Edmondson]

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.