[v2] Support fd-based KVM stats

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

Posted by Mark Kanda 4 years, 2 months ago

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")

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(+)

-- 
2.26.2

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

Posted by Paolo Bonzini 4 years ago

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(+)
>

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

Posted by Mark Kanda 4 years ago

(+ 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/ 
<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(+)
>>
>