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

From: Paolo Bonzini <pbonzini@redhat.com>
To: Mark Kanda <mark.kanda@oracle.com>, qemu-devel@nongnu.org
Subject: Re: [PATCH 0/2] Support fd-based KVM stats
Date: Wed, 20 Oct 2021 09:26:42 +0200	[thread overview]
Message-ID: <45a7173a-19e9-b5b8-7df2-0d494e4af3b6@redhat.com> (raw)
In-Reply-To: <20211019202904.4857-1-mark.kanda@oracle.com>

On 19/10/21 22:29, Mark Kanda wrote:
> This patchset adds QEMU support for querying fd-based KVM stats. The kernel
> support is provided by:
> 
> cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")
> 
> Patch 1 adds QMP support; patch 2 adds HMP support.

Hi Mark,

that's awesome, thanks!

Just a couple remarks on the implementation:

1) for QMP, it would be nice to have separate commands for the schema 
and the value.  This is because in the future we could consider passing 
the file descriptor directly to the QMP client; in which case, it would 
still be nicer to have QEMU turn the schema information into something 
more easily consumable.

2) for HMP, it would be nice to have the exponent converted to e.g. 
"nanoseconds" if the KvmStatUnit is seconds.  It's not super important, 
but it would be nice.  Something as simple as

if (kvm_stat->unit == KVM_STAT_UNIT_SECONDS &&
     kvm_stat->exponent >= -9 && kvm_stat->exponent <= 0 &&
     kvm_stat->exponent % 3 == 0 && kvm_stat->base == 10) {
     const char *si_prefix[] = { "", "milli", "micro", "nano" };
     monitor_printf(" %lu %sseconds", val->value,
                    si_prefix[kvm_stat->exponent / -3]);
} else ...

(Yes, this is nitpicking; but in practice nanoseconds are the only case 
where currently the exponent is not 0, so...).

3) more recent versions of Linux also support histograms, for which you 
have to include the bucket size and the kind (logarithmic/linear) in 
QMP.  For HMP it would be nice to have it printed as

     halt_wait_hist: [1 nanosecond]=200 [2]=30 [4]=50 [8]=120

but, for the first version, it's also okay if the QMP commands just 
skips unknown KVM_STATS_TYPE values altogether.

Paolo

> Mark Kanda (2):
>    qmp: Support fd-based KVM stats query
>    hmp: Support fd-based KVM stats query
> 
>   accel/kvm/kvm-all.c   | 246 ++++++++++++++++++++++++++++++++++++++++++
>   hmp-commands-info.hx  |  13 +++
>   include/monitor/hmp.h |   1 +
>   monitor/hmp-cmds.c    |  52 +++++++++
>   qapi/misc.json        |  73 +++++++++++++
>   5 files changed, 385 insertions(+)
> 