Re: [RFC PATCH 2/4] KVM: stats: Define APIs for aggregated stats retrieval in binary format

[Marc Zyngier <maz@kernel.org>]

On Wed, 10 Mar 2021 00:30:22 +0000,
Jing Zhang <jingzhangos@google.com> wrote:
> 
> Define ioctl commands for VM/vCPU aggregated statistics data retrieval
> in binary format and update corresponding API documentation.
> 
> The capability and ioctl are not enabled for now.
> No functional change intended.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  Documentation/virt/kvm/api.rst | 79 ++++++++++++++++++++++++++++++++++
>  include/linux/kvm_host.h       |  1 -
>  include/uapi/linux/kvm.h       | 60 ++++++++++++++++++++++++++
>  3 files changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 1a2b5210cdbf..aa4b5270c966 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -4938,6 +4938,76 @@ see KVM_XEN_VCPU_SET_ATTR above.
>  The KVM_XEN_VCPU_ATTR_TYPE_RUNSTATE_ADJUST type may not be used
>  with the KVM_XEN_VCPU_GET_ATTR ioctl.
>  
> +4.131 KVM_STATS_GET_INFO
> +------------------------
> +
> +:Capability: KVM_CAP_STATS_BINARY_FORM
> +:Architectures: all
> +:Type: vm ioctl, vcpu ioctl
> +:Parameters: struct kvm_stats_info (out)
> +:Returns: 0 on success, < 0 on error


Missing description of the errors (this is throughout the document).

> +
> +::
> +
> +  struct kvm_stats_info {
> +        __u32 num_stats;
> +  };
> +
> +This ioctl is used to get the information about VM or vCPU statistics data.
> +The number of statistics data would be returned in field num_stats in
> +struct kvm_stats_info. This ioctl only needs to be called once on running
> +VMs on the same architecture.

Is this allowed to be variable across VMs? Or is that a constant for a
given host system boot?

Given that this returns a single field, is there any value in copying
this structure across? Could it be returned by the ioctl itself
instead, at the expense of only being a 31bit value?

> +
> +4.132 KVM_STATS_GET_NAMES
> +-------------------------
> +
> +:Capability: KVM_CAP_STATS_BINARY_FORM
> +:Architectures: all
> +:Type: vm ioctl, vcpu ioctl
> +:Parameters: struct kvm_stats_names (in/out)
> +:Returns: 0 on success, < 0 on error
> +
> +::
> +
> +  #define KVM_STATS_NAME_LEN		32
> +  struct kvm_stats_names {
> +	__u32 size;
> +	__u8  names[0];
> +  };
> +
> +This ioctl is used to get the string names of all the statistics data for VM
> +or vCPU. Users must use KVM_STATS_GET_INFO to find the number of statistics.
> +They must allocate a buffer of the size num_stats * KVM_STATS_NAME_LEN
> +immediately following struct kvm_stats_names. The size field of kvm_stats_name
> +must contain the buffer size as an input.

What is the unit for the buffer size? bytes? or number of "names"?

> +The buffer can be treated like a string array, each name string is null-padded
> +to a size of KVM_STATS_NAME_LEN;

Is this allowed to query fewer strings than described by
kvm_stats_info? If it isn't, I question the need for the "size"
field. Either there is enough space in the buffer passed by userspace,
or -EFAULT is returned.

> +This ioclt only needs to be called once on running VMs on the same architecture.

Same question about the immutability of these names.

> +
> +4.133 KVM_STATS_GET_DATA
> +-------------------------
> +
> +:Capability: KVM_CAP_STATS_BINARY_FORM
> +:Architectures: all
> +:Type: vm ioctl, vcpu ioctl
> +:Parameters: struct kvm_stats_data (in/out)
> +:Returns: 0 on success, < 0 on error
> +
> +::
> +
> +  struct kvm_stats_data {
> +	__u32 size;

Same question about the actual need for this field.

> +	__u64 data[0];

So userspace always sees a 64bit quantify per stat. My earlier comment
about the ulong/u64 discrepancy stands! ;-)

> +  };
> +
> +This ioctl is used to get the aggregated statistics data for VM or vCPU.
> +Users must use KVM_STATS_GET_INFO to find the number of statistics.
> +They must allocate a buffer of the appropriate size num_stats * sizeof(data[0])
> +immediately following struct kvm_stats_data. The size field of kvm_stats_data
> +must contain the buffer size as an input.
> +The data buffer 1-1 maps to name strings buffer in sequential order.
> +This ioctl is usually called periodically to pull statistics data.

Is there any provision to reset the counters on read?

> +
>  5. The kvm_run structure
>  ========================
>  
> @@ -6721,3 +6791,12 @@ vcpu_info is set.
>  The KVM_XEN_HVM_CONFIG_RUNSTATE flag indicates that the runstate-related
>  features KVM_XEN_VCPU_ATTR_TYPE_RUNSTATE_ADDR/_CURRENT/_DATA/_ADJUST are
>  supported by the KVM_XEN_VCPU_SET_ATTR/KVM_XEN_VCPU_GET_ATTR ioctls.
> +
> +8.31 KVM_CAP_STATS_BINARY_FORM
> +------------------------------
> +
> +:Architectures: all
> +
> +This capability indicates that KVM supports retrieving aggregated statistics
> +data in binary format with corresponding VM/VCPU ioctl KVM_STATS_GET_INFO,
> +KVM_STATS_GET_NAMES and KVM_STATS_GET_DATA.

nit: for ease of reviewing, consider splitting the documentation in a
separate patch.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1ea297458306..f2dabf457717 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1164,7 +1164,6 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa)
>  
>  #define VM_STAT_COUNT		(sizeof(struct kvm_vm_stat)/sizeof(ulong))
>  #define VCPU_STAT_COUNT		(sizeof(struct kvm_vcpu_stat)/sizeof(u64))
> -#define KVM_STATS_NAME_LEN	32
>  
>  /* Make sure it is synced with fields in struct kvm_vm_stat. */
>  extern const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN];
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index f6afee209620..ad185d4c5e42 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1078,6 +1078,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_DIRTY_LOG_RING 192
>  #define KVM_CAP_X86_BUS_LOCK_EXIT 193
>  #define KVM_CAP_PPC_DAWR1 194
> +#define KVM_CAP_STATS_BINARY_FORM 195
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -1853,4 +1854,63 @@ struct kvm_dirty_gfn {
>  #define KVM_BUS_LOCK_DETECTION_OFF             (1 << 0)
>  #define KVM_BUS_LOCK_DETECTION_EXIT            (1 << 1)
>  
> +/* Available with KVM_CAP_STATS_BINARY_FORM */
> +
> +#define KVM_STATS_NAME_LEN		32
> +
> +/**
> + * struct kvm_stats_info - statistics information
> + *
> + * Used as parameter for ioctl %KVM_STATS_GET_INFO.
> + *
> + * @num_stats: On return, the number of statistics data of vm or vcpu.
> + *
> + */
> +struct kvm_stats_info {
> +	__u32 num_stats;
> +};
> +
> +/**
> + * struct kvm_stats_names - string list of statistics names
> + *
> + * Used as parameter for ioctl %KVM_STATS_GET_NAMES.
> + *
> + * @size: Input from user, indicating the size of buffer after the struture.
> + * @names: Buffer of name string list for vm or vcpu. Each string is
> + *	null-padded to a size of %KVM_STATS_NAME_LEN.
> + *
> + * Users must use %KVM_STATS_GET_INFO to find the number of
> + * statistics. They must allocate a buffer of the appropriate
> + * size (>= &struct kvm_stats_info @num_stats * %KVM_STATS_NAME_LEN)
> + * immediately following this struture.
> + */
> +struct kvm_stats_names {
> +	__u32 size;
> +	__u8  names[0];
> +};
> +
> +/**
> + * struct kvm_stats_data - statistics data array
> + *
> + * Used as parameter for ioctl %KVM_STATS_GET_DATA.
> + *
> + * @size: Input from user, indicating the size of buffer after the struture.
> + * @data: Buffer of statistics data for vm or vcpu.
> + *
> + * Users must use %KVM_STATS_GET_INFO to find the number of
> + * statistics. They must allocate a buffer of the appropriate
> + * size (>= &struct kvm_stats_info @num_stats * sizeof(@data[0])
> + * immediately following this structure.
> + * &struct kvm_stats_names @names 1-1 maps to &structkvm_stats_data
> + * @data in sequential order.
> + */
> +struct kvm_stats_data {
> +	__u32 size;
> +	__u64 data[0];
> +};
> +
> +#define KVM_STATS_GET_INFO		_IOR(KVMIO, 0xcc, struct kvm_stats_info)
> +#define KVM_STATS_GET_NAMES		_IOR(KVMIO, 0xcd, struct kvm_stats_names)
> +#define KVM_STATS_GET_DATA		_IOR(KVMIO, 0xce, struct kvm_stats_data)
> +
>  #endif /* __LINUX_KVM_H */
> -- 
> 2.30.1.766.gb4fecdf3b7-goog
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.