Re: [PATCH v2 0/3] Support fd-based KVM stats

[Mark Kanda]

    (+ Daniel and Markus)
    
    Thank you Paolo,

I'm in the midst of implementing various API changes as requested by Daniel [1] and was planning to send out v3 this week. Could you please take a look at his response and comment on the proposal? Or, perhaps I should publish v3 (based on Daniel's proposal) and you could review that? Please let me know your preference.

Thanks/regards,

-Mark

[1] https://lore.kernel.org/qemu-devel/Ya+rLex1djU%2F1Wc1@redhat.com/

On 1/15/2022 10:22 AM, Paolo Bonzini wrote:

> On
>       11/19/21 20:51, Mark Kanda wrote:
>       
> > v2: [Paolo]
> >         
> >         - generalize the interface
> >         
> >         - add support for querying stat schema and instances
> >         
> >         - add additional HMP semantic processing for a few exponent/unit
> >         
> >            combinations (related to seconds and bytes)
> >         
> >         
> >         This patchset adds QEMU support for querying fd-based KVM stats.
> >         The
> >         
> >         kernel support was introduced by:
> >         
> >         
> >         cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats
> >         data")
> >         
> >       
>       
>       Hi Mark,
>       
>       
>       sorry for the late review.  Fortunately there's very little that
>       I'd change.
>       
>       
>       In particular:
>       
>       
>       * please change the callbacks to accept a NULL name and type,
>       instead of having the "bool"/"const char *" pair.  You can
>       probably benefit from a function to cutils.c like
>       
>       
>           bool qemu_match_string(const char *value, const char *request)
>       {
>       
>               return !request || g_str_equal(value, request);
>       
>           }
>       
>       
>       * please pass a single const struct to add_stats_callbacks, using
>       GList so that the struct can be const.
>       
>       
>       Putting both together it would be something like:
>       
>       
>       typedef struct StatsCallbacks {
>       
>           char *name;
>       
>           StatsList *(*get_values)(StatsList *list, const char *name,
>       
>                                  const char *type, Error **errp);
>       
>           StatsSchemaList *(*get_schemas)(StatsSchemaList *list,
>       
>                                           const char *name, Error
>       **errp);
>       
>           StatsInstanceList *(*get_instances)(StatsInstanceList *list,
>       
>                                               Error **errp);
>       
>       } StatsCallbacks;
>       
>       
>       Finally, please put everything in a new header
>       include/monitor/stats.h, so that we can document everything and
>       please it in docs/devel.  I can take care of that though.
>       
>       
>       Thanks,
>       
>       
>       Paolo
>       
>       
> >         
> >         Mark Kanda (3):
> >         
> >            qmp: Support for querying stats
> >         
> >            hmp: Support for querying stats
> >         
> >            kvm: Support for querying fd-based stats
> >         
> >         
> >           accel/kvm/kvm-all.c       | 399
> >         ++++++++++++++++++++++++++++++++++++++
> >         
> >           hmp-commands-info.hx      |  40 ++++
> >         
> >           include/monitor/hmp.h     |   3 +
> >         
> >           include/monitor/monitor.h |  27 +++
> >         
> >           monitor/hmp-cmds.c        | 125 ++++++++++++
> >         
> >           monitor/qmp-cmds.c        |  71 +++++++
> >         
> >           qapi/misc.json            | 142 ++++++++++++++
> >         
> >           7 files changed, 807 insertions(+)
> >         
> >         
> >       
>       
>