Hi Marc,
On Wed, Mar 10, 2021 at 8:19 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Hi Jing,
>
> On Wed, 10 Mar 2021 00:30:21 +0000,
> Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> >
> > Prepare the statistics name strings for supporting binary format
> > aggregated statistics data retrieval.
> >
> > No functional change intended.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/guest.c | 47 ++++--
> > arch/mips/kvm/mips.c | 114 ++++++++++----
> > arch/powerpc/kvm/book3s.c | 107 +++++++++----
> > arch/powerpc/kvm/booke.c | 84 +++++++---
> > arch/s390/kvm/kvm-s390.c | 320 ++++++++++++++++++++++++++------------
> > arch/x86/kvm/x86.c | 127 ++++++++++-----
> > include/linux/kvm_host.h | 31 +++-
> > 7 files changed, 589 insertions(+), 241 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 9bbd30e62799..fb3aafe76b52 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -28,19 +28,42 @@
> >
> > #include "trace.h"
> >
> > +const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN] = {
> > + "remote_tlb_flush",
> > +};
> > +static_assert(sizeof(kvm_vm_stat_strings) ==
> > + VM_STAT_COUNT * KVM_STATS_NAME_LEN);
> > +
> > +const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN] = {
> > + "halt_successful_poll",
> > + "halt_attempted_poll",
> > + "halt_poll_success_ns",
> > + "halt_poll_fail_ns",
> > + "halt_poll_invalid",
> > + "halt_wakeup",
> > + "hvc_exit_stat",
> > + "wfe_exit_stat",
> > + "wfi_exit_stat",
> > + "mmio_exit_user",
> > + "mmio_exit_kernel",
> > + "exits",
> > +};
> > +static_assert(sizeof(kvm_vcpu_stat_strings) ==
> > + VCPU_STAT_COUNT * KVM_STATS_NAME_LEN);
> > +
> > struct kvm_stats_debugfs_item debugfs_entries[] = {
> > - VCPU_STAT("halt_successful_poll", halt_successful_poll),
> > - VCPU_STAT("halt_attempted_poll", halt_attempted_poll),
> > - VCPU_STAT("halt_poll_invalid", halt_poll_invalid),
> > - VCPU_STAT("halt_wakeup", halt_wakeup),
> > - VCPU_STAT("hvc_exit_stat", hvc_exit_stat),
> > - VCPU_STAT("wfe_exit_stat", wfe_exit_stat),
> > - VCPU_STAT("wfi_exit_stat", wfi_exit_stat),
> > - VCPU_STAT("mmio_exit_user", mmio_exit_user),
> > - VCPU_STAT("mmio_exit_kernel", mmio_exit_kernel),
> > - VCPU_STAT("exits", exits),
> > - VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns),
> > - VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns),
> > + VCPU_STAT(halt_successful_poll),
> > + VCPU_STAT(halt_attempted_poll),
> > + VCPU_STAT(halt_poll_invalid),
> > + VCPU_STAT(halt_wakeup),
> > + VCPU_STAT(hvc_exit_stat),
> > + VCPU_STAT(wfe_exit_stat),
> > + VCPU_STAT(wfi_exit_stat),
> > + VCPU_STAT(mmio_exit_user),
> > + VCPU_STAT(mmio_exit_kernel),
> > + VCPU_STAT(exits),
> > + VCPU_STAT(halt_poll_success_ns),
> > + VCPU_STAT(halt_poll_fail_ns),
>
> So we now have two arrays that can easily deviate in their order,
> whilst we didn't have that risk before. What is the advantage of doing
> this? The commit message doesn't really say...
You are right about the risk here. The new string array would be returned
to userspace by the new Ioctl API. I didn't figure out any other good
way for this.
Will add this into commit message.
>
> [...]
>
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 1b65e7204344..1ea297458306 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1162,6 +1162,18 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa)
> > return kvm_is_error_hva(hva);
> > }
> >
> > +#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];
> > +/* Make sure it is synced with fields in struct kvm_vcpu_stat. */
> > +extern const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN];
> > +
> > +#define VM_STAT_NAME(id) (kvm_vm_stat_strings[id])
> > +#define VCPU_STAT_NAME(id) (kvm_vcpu_stat_strings[id])
> > +
> > enum kvm_stat_kind {
> > KVM_STAT_VM,
> > KVM_STAT_VCPU,
> > @@ -1182,10 +1194,21 @@ struct kvm_stats_debugfs_item {
> > #define KVM_DBGFS_GET_MODE(dbgfs_item) \
> > ((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644)
> >
> > -#define VM_STAT(n, x, ...) \
> > - { n, offsetof(struct kvm, stat.x), KVM_STAT_VM, ## __VA_ARGS__ }
> > -#define VCPU_STAT(n, x, ...) \
> > - { n, offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU, ## __VA_ARGS__ }
> > +#define VM_STAT(x, ...) \
> > + { \
> > + VM_STAT_NAME(offsetof(struct kvm_vm_stat, x)/sizeof(ulong)), \
> > + offsetof(struct kvm, stat.x), \
> > + KVM_STAT_VM, \
> > + ## __VA_ARGS__ \
> > + }
> > +
> > +#define VCPU_STAT(x, ...) \
> > + { \
> > + VCPU_STAT_NAME(offsetof(struct kvm_vcpu_stat, x)/sizeof(u64)), \
> > + offsetof(struct kvm_vcpu, stat.x), \
> > + KVM_STAT_VCPU, \
> > + ## __VA_ARGS__ \
> > + }
>
> Is there any reason why we want to keep kvm_vm_stat populated with
> ulong, while kvm_vcpu_stat is populated with u64? I have the feeling
> that this is a fairly pointless difference, and that some of the
> macros could be unified.
The use of ulong for vm stats is to avoid the overhead of atomics,
since vm stats
could potentially be updated by multiple vcpus from that vm at a time.
Check commit 8a7e75d47b68193339f8727cf4503271d0a0b1d0 for details.
>
> Also, using names initialisers would help with the readability of the
> macros.
Sure, will do.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing
