Re: [PATCH v5 07/10] qmp: add filtering of statistics by provider

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Allow retrieving the statistics from a specific provider only.
> This can be used in the future by HMP commands such as "info
> sync-profile" or "info profile".  The next patch also adds
> filter-by-provider capabilities to the HMP equivalent of
> query-stats, "info stats".
> 
> Example:
> 
> { "execute": "query-stats",
>   "arguments": {
>     "target": "vm",
>     "providers": [
>       { "provider": "kvm" } ] } }
> 
> The QAPI is a bit more verbose than just a list of StatsProvider,
> so that it can be subsequently extended with filtering of statistics
> by name.
> 
> If a provider is specified more than once in the filter, each request
> will be included separately in the output.
> 
> Extracted from a patch by Mark Kanda.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>         Here I removed the Reviewed-by because the effect of the new
>         implementation is especially marekd when you have multiple filters.
>         Before, all filters were evaluated first, and the callback was then
>         run if there was a match.  Now, each filter is evaluated separately.
> 
>  accel/kvm/kvm-all.c     |  3 ++-
>  include/monitor/stats.h |  4 +++-
>  monitor/hmp-cmds.c      |  2 +-
>  monitor/qmp-cmds.c      | 41 ++++++++++++++++++++++++++++++++---------
>  qapi/stats.json         | 19 +++++++++++++++++--
>  5 files changed, 55 insertions(+), 14 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index d75fb3d95c..66c4ac1ac6 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2644,7 +2644,8 @@ static int kvm_init(MachineState *ms)
>      }
>  
>      if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
> -        add_stats_callbacks(query_stats_cb, query_stats_schemas_cb);
> +        add_stats_callbacks(STATS_PROVIDER_KVM, query_stats_cb,
> +                            query_stats_schemas_cb);
>      }
>  
>      return 0;
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> index 8c50feeaa9..80a523dd29 100644
> --- a/include/monitor/stats.h
> +++ b/include/monitor/stats.h
> @@ -17,10 +17,12 @@ typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
>  /*
>   * Register callbacks for the QMP query-stats command.
>   *
> + * @provider: stats provider checked against QMP command arguments
>   * @stats_fn: routine to query stats:
>   * @schema_fn: routine to query stat schemas:
>   */
> -void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +void add_stats_callbacks(StatsProvider provider,
> +                         StatRetrieveFunc *stats_fn,
>                           SchemaRetrieveFunc *schemas_fn);
>  
>  /*
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 55b83c0a3a..c501d1fa2b 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2397,7 +2397,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
>          goto exit_no_print;
>      }
>  
> -    schema = qmp_query_stats_schemas(&err);
> +    schema = qmp_query_stats_schemas(false, STATS_PROVIDER__MAX, &err);
>      if (err) {
>          goto exit;
>      }
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 5f8f1e620b..e49ab345d7 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -445,6 +445,7 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
>  }
>  
>  typedef struct StatsCallbacks {
> +    StatsProvider provider;
>      StatRetrieveFunc *stats_cb;
>      SchemaRetrieveFunc *schemas_cb;
>      QTAILQ_ENTRY(StatsCallbacks) next;
> @@ -453,10 +454,12 @@ typedef struct StatsCallbacks {
>  static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
>      QTAILQ_HEAD_INITIALIZER(stats_callbacks);
>  
> -void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +void add_stats_callbacks(StatsProvider provider,
> +                         StatRetrieveFunc *stats_fn,
>                           SchemaRetrieveFunc *schemas_fn)
>  {
>      StatsCallbacks *entry = g_new(StatsCallbacks, 1);
> +    entry->provider = provider;
>      entry->stats_cb = stats_fn;
>      entry->schemas_cb = schemas_fn;
>  
> @@ -465,12 +468,18 @@ void add_stats_callbacks(StatRetrieveFunc *stats_fn,
>  
>  static bool invoke_stats_cb(StatsCallbacks *entry,
>                              StatsResultList **stats_results,
> -                            StatsFilter *filter,
> +                            StatsFilter *filter, StatsRequest *request,
>                              Error **errp)
>  {
>      strList *targets = NULL;
>      ERRP_GUARD();
>  
> +    if (request) {
> +        if (request->provider != entry->provider) {
> +            return true;
> +        }
> +    }
> +
>      switch (filter->target) {
>      case STATS_TARGET_VM:
>          break;
> @@ -500,27 +509,41 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
>  {
>      StatsResultList *stats_results = NULL;
>      StatsCallbacks *entry;
> +    StatsRequestList *request;
>  
>      QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> -        if (!invoke_stats_cb(entry, &stats_results, filter, errp)) {
> -            break;
> +        if (filter->has_providers) {
> +            for (request = filter->providers; request; request = request->next) {
> +                if (!invoke_stats_cb(entry, &stats_results, filter,
> +                                     request->value, errp)) {
> +                    break;
> +                }
> +            }
> +        } else {
> +            if (!invoke_stats_cb(entry, &stats_results, filter, NULL, errp)) {
> +                break;
> +            }
>          }
>      }
>  
>      return stats_results;
>  }
>  
> -StatsSchemaList *qmp_query_stats_schemas(Error **errp)
> +StatsSchemaList *qmp_query_stats_schemas(bool has_provider,
> +                                         StatsProvider provider,
> +                                         Error **errp)
>  {
>      StatsSchemaList *stats_results = NULL;
>      StatsCallbacks *entry;
>      ERRP_GUARD();
>  
>      QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> -        entry->schemas_cb(&stats_results, errp);
> -        if (*errp) {
> -            qapi_free_StatsSchemaList(stats_results);
> -            return NULL;
> +        if (!has_provider || provider == entry->provider) {
> +            entry->schemas_cb(&stats_results, errp);
> +            if (*errp) {
> +                qapi_free_StatsSchemaList(stats_results);
> +                return NULL;
> +            }
>          }
>      }
>  
> diff --git a/qapi/stats.json b/qapi/stats.json
> index 8c9abb57f1..503918ea4c 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -69,6 +69,18 @@
>  { 'enum': 'StatsTarget',
>    'data': [ 'vm', 'vcpu' ] }
>  
> +##
> +# @StatsRequest:
> +#
> +# Indicates a set of statistics that should be returned by query-stats.
> +#
> +# @provider: provider for which to return statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsRequest',
> +  'data': { 'provider': 'StatsProvider' } }
> +
>  ##
>  # @StatsVCPUFilter:
>  #
> @@ -86,11 +98,14 @@
>  # request statistics and optionally the required subset of information for
>  # that target:
>  # - which vCPUs to request statistics for
> +# - which providers to request statistics from
>  #
>  # Since: 7.1
>  ##
>  { 'union': 'StatsFilter',
> -        'base': { 'target': 'StatsTarget' },
> +  'base': {
> +      'target': 'StatsTarget',
> +      '*providers': [ 'StatsRequest' ] },
>    'discriminator': 'target',
>    'data': { 'vcpu': 'StatsVCPUFilter' } }
>  
> @@ -226,5 +241,5 @@
>  # Since: 7.1
>  ##
>  { 'command': 'query-stats-schemas',
> -  'data': { },
> +  'data': { '*provider': 'StatsProvider' },
>    'returns': [ 'StatsSchema' ] }
> -- 
> 2.36.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK