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

[Zheng Chuan <zhengchuan@huawei.com>]

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