On Tue, Mar 04, 2025 at 04:02:28PM +0100, Peter Krempa wrote:
> On Tue, Mar 04, 2025 at 14:04:04 +0000, Daniel P. Berrangé wrote:
> > Contrary to most APIs returning typed parameters, there are no constants
> > defined for the domain stats data keys. This is was because many of the
> > keys needs to be dynamically constructed using one or more array index
> > values.
> >
> > It is possible to define constants while still supporting dynamic
> > array indexes by simply defining the prefixes and suffixes as constants.
> > The consuming code can then combine the constants with array index
> > value.
> >
> > With this approach, it is practical to add constants for the domain stats
> > API keys.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > include/libvirt/libvirt-domain.h | 128 +++++++++++++++++++++++++++++++
> > src/libvirt-domain.c | 30 +-------
> > src/qemu/qemu_driver.c | 44 +++++++----
> > 3 files changed, 162 insertions(+), 40 deletions(-)
> >
> > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > index 5b014adcd0..7e9f998f2f 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -2802,6 +2802,134 @@ struct _virDomainStatsRecord {
> > */
> > #define VIR_DOMAIN_STATS_STATE_REASON "state.reason"
> >
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_TIME:
> > + *
> > + * Total cpu time spent for this domain in nanoseconds as unsigned long long.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_TIME "cpu.time"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_USER:
> > + *
> > + * User cpu time spent in nanoseconds as unsigned long long.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_USER "cpu.user"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_SYSTEM:
> > + *
> > + * System cpu time spent in nanoseconds as unsigned long long.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_SYSTEM "cpu.system"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_HALTPOLL_SUCCESS_TIME:
> > + *
> > + * Halt-polling cpu usage about the VCPU polled until a virtual interrupt was
> > + * delivered in nanoseconds as unsigned long long.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_HALTPOLL_SUCCESS_TIME "cpu.haltpoll.success.time"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_HALTPOLL_FAIL_TIME:
> > + *
> > + * Halt-polling cpu usage about the VCPU had to schedule out (either because
> > + * the maximum poll time was reached or it needed to yield the CPU) in
> > + * nanoseconds as unsigned long long.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_HALTPOLL_FAIL_TIME "cpu.haltpoll.fail.time"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_COUNT:
> > + *
> > + * The number of cache monitors for this domain as an unsigned int.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_COUNT "cpu.cache.monitor.count"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_PREFIX:
> > + *
> > + * The parameter name prefix to access each cache monitor entry. Concatenate
> > + * the prefix, the entry number formatted as an unsigned integer and one of
> > + * the cache monitor suffix parameters to form a complete parameter name.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_PREFIX "cpu.cache.monitor."
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_NAME:
> > + *
> > + * The name of cache monitor as a string.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_NAME ".name"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_VCPUS:
> > + *
> > + * Vcpu list of cache monitor as a string.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_VCPUS ".vcpus"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_COUNT:
> > + *
> > + * The number of cache banks in cache monitor as an unsigned int.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_COUNT ".bank.count"
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_PREFIX:
> > + *
> > + * The parameter name prefix to access each cache monitor bank entry.
> > + * Concatenate the cache monitor prefix, the cache monitor entry number
> > + * formatted as an unsigned integer, the bank prefix, the bank entry number
> > + * formatted as an unsigned integer and one of the cache monitor bank suffix
> > + * parameters to form a complete parameter name.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_PREFIX ".bank."
> > +
> > +/**
> > + * VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_SUFFIX_ID:
> > + *
> > + * Host allocated cache id for the bank as an unsigned int.
> > + *
> > + * Since: 11.2.0
> > + */
> > +#define VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_SUFFIX_ID ".id"
>
>
> I really consider this to be much less understandable if you are
> attempting to figure out what this API returns.
>
> At least for the final suffix you should add the example as it was
> formatted in the docs you're replacing. Alternatively with actual
> numbers instead of the substitution tokens such as <num>.
Perhaps the example usage from my cover letter should be added in the API
docs for the guest info / stats APIs.
> > for (j = 0; j < resdata[i]->nstats; j++) {
> > - virTypedParamListAddUInt(params, resdata[i]->stats[j]->id,
> > - "cpu.cache.monitor.%zu.bank.%zu.id", i, j);
> > + virTypedParamListAddUInt(
> > + params, resdata[i]->stats[j]->id,
> > + VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_PREFIX "%zu"
> > + VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_PREFIX "%zu"
> > + VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_SUFFIX_ID, i, j);
>
> I'd prefer if you not do this style of alignment. We mention in the
> coding style docs that doing weird alignment is unwanted if done purely
> to stick to "80 columns".
Yeah, I was in two minds over whether to leverage that rule in
this case, since the lines would end up pretty significantly
over 80.
If folks are ok with real long lines, i'll unwrap all the
constant usage.
> > + virTypedParamListAddUInt(
> > + params, (unsigned int)resdata[i]->stats[j]->vals[0],
> > + VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_PREFIX "%zu"
> > + VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_PREFIX "%zu"
> > + VIR_DOMAIN_STATS_CPU_CACHE_MONITOR_SUFFIX_BANK_SUFFIX_BYTES, i, j);
>
> here too.
....and in many following patches
> > }
> > }
>
> Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
