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