Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Allow retrieving only a subset of statistics.  This can be useful
> for example in order to plot a subset of the statistics many times
> a second.
>
> KVM publishes ~40 statistics for each vCPU on x86; retrieving and
> serializing all of them would be useless
>
> Another use will be in HMP in the following patch; implementing the
> filter in the backend is easy enough that it was deemed okay to make
> this a public interface.
>
> Example:
>
> { "execute": "query-stats",
>   "arguments": {
>     "target": "vcpu",
>     "vcpus": [ "/machine/unattached/device[2]",
>                "/machine/unattached/device[4]" ],
>     "providers": [
>       { "provider": "kvm",
>         "names": [ "l1d_flush", "exits" ] } } }
>
> { "return": {
>     "vcpus": [
>       { "path": "/machine/unattached/device[2]"
>         "providers": [
>           { "provider": "kvm",
>             "stats": [ { "name": "l1d_flush", "value": 41213 },
>                        { "name": "exits", "value": 74291 } ] } ] },
>       { "path": "/machine/unattached/device[4]"
>         "providers": [
>           { "provider": "kvm",
>             "stats": [ { "name": "l1d_flush", "value": 16132 },
>                        { "name": "exits", "value": 57922 } ] } ] } ] } }
>
> Extracted from a patch by Mark Kanda.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v3->v4: handle empty filter early by avoiding the query altogether
>
>  accel/kvm/kvm-all.c     | 17 +++++++++++------
>  include/monitor/stats.h |  4 ++--
>  monitor/qmp-cmds.c      | 16 +++++++++++++---
>  qapi/stats.json         |  6 +++++-
>  4 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 461b6cf927..a61d17719e 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2311,7 +2311,7 @@ bool kvm_dirty_ring_enabled(void)
>      return kvm_state->kvm_dirty_ring_size ? true : false;
>  }
>  
> -static void query_stats_cb(StatsResultList **result, StatsTarget target,
> +static void query_stats_cb(StatsResultList **result, StatsTarget target, strList *names,

Long line.

>                             strList *targets, Error **errp);
>  static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
>  
> @@ -3713,6 +3713,7 @@ typedef struct StatsArgs {
>          StatsResultList **stats;
>          StatsSchemaList **schema;
>      } result;
> +    strList *names;
>      Error **errp;
>  } StatsArgs;
>  
> @@ -3926,7 +3927,7 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
>      return descriptors;
>  }
>  
> -static void query_stats(StatsResultList **result, StatsTarget target,
> +static void query_stats(StatsResultList **result, StatsTarget target, strList *names,

Long line.

>                          int stats_fd, Error **errp)
>  {
>      struct kvm_stats_desc *kvm_stats_desc;
> @@ -3969,6 +3970,9 @@ static void query_stats(StatsResultList **result, StatsTarget target,
>  
>          /* Add entry to the list */
>          stats = (void *)stats_data + pdesc->offset;
> +        if (!apply_str_list_filter(pdesc->name, names)) {
> +            continue;
> +        }
>          stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
>      }
>  
> @@ -4030,8 +4034,8 @@ static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
>          error_propagate(kvm_stats_args->errp, local_err);
>          return;
>      }
> -    query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
> -                kvm_stats_args->errp);
> +    query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU,
> +                kvm_stats_args->names, stats_fd, kvm_stats_args->errp);
>      close(stats_fd);
>  }
>  
> @@ -4052,7 +4056,7 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
>  }
>  
>  static void query_stats_cb(StatsResultList **result, StatsTarget target,
> -                           strList *targets, Error **errp)
> +                           strList *names, strList *targets, Error **errp)
>  {
>      KVMState *s = kvm_state;
>      CPUState *cpu;
> @@ -4066,7 +4070,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
>              error_setg(errp, "KVM stats: ioctl failed");
>              return;
>          }
> -        query_stats(result, target, stats_fd, errp);
> +        query_stats(result, target, names, stats_fd, errp);
>          close(stats_fd);
>          break;
>      }
> @@ -4074,6 +4078,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
>      {
>          StatsArgs stats_args;
>          stats_args.result.stats = result;
> +        stats_args.names = names;
>          stats_args.errp = errp;
>          CPU_FOREACH(cpu) {
>              if (!apply_str_list_filter(cpu->parent_obj.canonical_path, targets)) {
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> index 840c4ed08e..88f046f568 100644
> --- a/include/monitor/stats.h
> +++ b/include/monitor/stats.h
> @@ -11,8 +11,8 @@
>  #include "qapi/qapi-types-stats.h"
>  
>  typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
> -                              strList *targets, Error **errp);
> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> +                              strList *names, strList *targets, Error **errp);
> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);

Did you drop the parameter names intentionally?

>  
>  /*
>   * Register callbacks for the QMP query-stats command.
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 7be0e7414a..e822f1b0a9 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -467,15 +467,24 @@ void add_stats_callbacks(StatsProvider provider,
>  }
>  
>  static bool stats_provider_requested(StatsProvider provider,
> -                                     StatsFilter *filter)
> +                                     StatsFilter *filter,
> +                                     strList **p_names)
>  {
>      StatsRequestList *request;
>  
> +    *p_names = NULL;
>      if (!filter->has_providers) {
>          return true;
>      }
>      for (request = filter->providers; request; request = request->next) {
>          if (request->value->provider == provider) {
> +            if (request->value->has_names) {
> +                if (!request->value->names) {
> +                    /* No names allowed is the same as skipping the provider.  */

Long line.

> +                    return false;

Any other elements of filter->providers that match @provider will be
silently ignored.  Is this what you want?

Uh, do we leak @p_names if earlier elements matched?

> +                }
> +                *p_names = request->value->names;
> +            }
>              return true;
>          }
>      }
> @@ -506,8 +515,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
>      }
>  
>      QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> -        if (stats_provider_requested(entry->provider, filter)) {
> -            entry->stats_cb(&stats_results, filter->target, targets, errp);
> +        strList *names = NULL;
> +        if (stats_provider_requested(entry->provider, filter, &names)) {
> +            entry->stats_cb(&stats_results, filter->target, names, targets, errp);

Brain cramp (broken night)... where is @names freed?

Long line.

>              if (*errp) {
>                  qapi_free_StatsResultList(stats_results);
>                  return NULL;
> diff --git a/qapi/stats.json b/qapi/stats.json
> index b75bb86124..2dbf188d36 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -74,11 +74,14 @@
>  # Indicates a set of statistics that should be returned by query-stats.
>  #
>  # @provider: provider for which to return statistics.
> +
> +# @names: statistics to be returned (all if omitted).
>  #
>  # Since: 7.1
>  ##
>  { 'struct': 'StatsRequest',
> -  'data': { 'provider': 'StatsProvider' } }
> +  'data': { 'provider': 'StatsProvider',
> +            '*names': [ 'str' ] } }
>  
>  ##
>  # @StatsVCPUFilter:
> @@ -98,6 +101,7 @@
>  # that target:
>  # - which vCPUs to request statistics for
>  # - which provider to request statistics from

Should this be "which providers"?

> +# - which values to return within each provider

This is member @provider sub-member @names.  Hmm.  "which named values
to return"?

>  #
>  # Since: 7.1
>  ##