| Message ID | 20210618044819.3690166-1-jingzhangos@google.com |
|---|---|
| Headers | show |
| Series | KVM statistics data fd-based binary interface | expand |
On Fri, Jun 18, 2021 at 04:48:15AM +0000, Jing Zhang wrote: > Add a VM ioctl to get a statistics file descriptor by which a read > functionality is provided for userspace to read out VM stats header, > descriptors and data. > Define VM statistics descriptors and header for all architectures. > > Reviewed-by: David Matlack <dmatlack@google.com> > Reviewed-by: Ricardo Koller <ricarkol@google.com> > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Fuad Tabba <tabba@google.com> > Tested-by: Fuad Tabba <tabba@google.com> #arm64 > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > arch/arm64/kvm/guest.c | 14 +++++++++++++ > arch/mips/kvm/mips.c | 14 +++++++++++++ > arch/powerpc/kvm/book3s.c | 16 +++++++++++++++ > arch/powerpc/kvm/booke.c | 16 +++++++++++++++ > arch/s390/kvm/kvm-s390.c | 19 +++++++++++++++++ > arch/x86/kvm/x86.c | 24 ++++++++++++++++++++++ > include/linux/kvm_host.h | 6 ++++++ > virt/kvm/kvm_main.c | 43 +++++++++++++++++++++++++++++++++++++++ > 8 files changed, 152 insertions(+) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 4962331d01e6..f456d1defe2b 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -28,6 +28,20 @@ > > #include "trace.h" > > +struct _kvm_stats_desc kvm_vm_stats_desc[] = { > + KVM_GENERIC_VM_STATS() > +}; > +static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > + sizeof(struct kvm_vm_stat) / sizeof(u64)); > + > +struct kvm_stats_header kvm_vm_stats_header = { Can this be const? > + .name_size = KVM_STATS_NAME_LEN, > + .count = ARRAY_SIZE(kvm_vm_stats_desc), > + .desc_offset = sizeof(struct kvm_stats_header) + KVM_STATS_ID_MAXLEN, > + .data_offset = sizeof(struct kvm_stats_header) + KVM_STATS_ID_MAXLEN + > + sizeof(kvm_vm_stats_desc), > +}; If it can't be const, what is modified in it that prevents that from happening? thanks, greg k-h
On Fri, Jun 18, 2021 at 04:48:17AM +0000, Jing Zhang wrote: > This new API provides a file descriptor for every VM and VCPU to read > KVM statistics data in binary format. > It is meant to provide a lightweight, flexible, scalable and efficient > lock-free solution for user space telemetry applications to pull the > statistics data periodically for large scale systems. The pulling > frequency could be as high as a few times per second. > The statistics descriptors are defined by KVM in kernel and can be > by userspace to discover VM/VCPU statistics during the one-time setup > stage. > The statistics data itself could be read out by userspace telemetry > periodically without any extra parsing or setup effort. > There are a few existed interface protocols and definitions, but no > one can fulfil all the requirements this interface implemented as > below: > 1. During high frequency periodic stats reading, there should be no > extra efforts except the stats data read itself. > 2. Support stats annotation, like type (cumulative, instantaneous, > peak, histogram, etc) and unit (counter, time, size, cycles, etc). > 3. The stats data reading should be free of lock/synchronization. We > don't care about the consistency between all the stats data. All > stats data can not be read out at exactly the same time. We really > care about the change or trend of the stats data. The lock-free > solution is not just for efficiency and scalability, also for the > stats data accuracy and usability. For example, in the situation > that all the stats data readings are protected by a global lock, > if one VCPU died somehow with that lock held, then all stats data > reading would be blocked, then we have no way from stats data that > which VCPU has died. > 4. The stats data reading workload can be handed over to other > unprivileged process. > > Reviewed-by: David Matlack <dmatlack@google.com> > Reviewed-by: Ricardo Koller <ricarkol@google.com> > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Fuad Tabba <tabba@google.com> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > Documentation/virt/kvm/api.rst | 176 ++++++++++++++++++++++++++++++++- > 1 file changed, 175 insertions(+), 1 deletion(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index e328caa35d6c..7ca1c8d190c0 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -5034,7 +5034,6 @@ 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_GET_SREGS2 > ------------------ > > @@ -5081,6 +5080,173 @@ Writes special registers into the vcpu. > See KVM_GET_SREGS2 for the data structures. > This ioctl (when supported) replaces the KVM_SET_SREGS. > > +4.133 KVM_GET_STATS_FD > +---------------------- > + > +:Capability: KVM_CAP_STATS_BINARY_FD > +:Architectures: all > +:Type: vm ioctl, vcpu ioctl > +:Parameters: none > +:Returns: statistics file descriptor on success, < 0 on error > + > +Errors: > + > + ====== ====================================================== > + ENOMEM if the fd could not be created due to lack of memory > + EMFILE if the number of opened files exceeds the limit > + ====== ====================================================== > + > +The file descriptor can be used to read VM/vCPU statistics data in binary > +format. The file data is organized into three blocks as below: > ++-------------+ > +| Header | > ++-------------+ > +| Descriptors | > ++-------------+ > +| Stats Data | > ++-------------+ > + > +The Header block is always at the start of the file. It is only needed to be > +read one time for the lifetime of the file descriptor. > +It is in the form of ``struct kvm_stats_header`` as below:: > + > + #define KVM_STATS_ID_MAXLEN 64 > + > + struct kvm_stats_header { > + __u32 name_size; > + __u32 count; > + __u32 desc_offset; > + __u32 data_offset; > + char id[]; > + }; > + > +The ``id`` field is a '\0' terminated string which identifies the corresponding > +KVM statistics. For VM statistics, it is in the form of "kvm-{kvm pid}", like > +"kvm-12345". For VCPU statistics, it is in the form of > +"kvm-{kvm pid}/vcpu-{vcpu id}", like "kvm-12345/vcpu-12". > + > +The ``name_size`` field is the size (in byte) of the statistics name string > +(including trailing '\0') appended to the end of every statistics descriptor. > + > +The ``count`` field is the number of statistics. > + > +The ``desc_offset`` field is the offset of the Descriptors block from the start > +of the file indicated by the file descriptor. > + > +The ``data_offset`` field is the offset of the Stats Data block from the start > +of the file indicated by the file descriptor. > + > +The Descriptors block is only needed to be read once for the lifetime of the > +file descriptor. It is an array of ``struct kvm_stats_desc`` as shown in > +below code block:: > + > + #define KVM_STATS_TYPE_SHIFT 0 > + #define KVM_STATS_TYPE_MASK (0xF << KVM_STATS_TYPE_SHIFT) > + #define KVM_STATS_TYPE_CUMULATIVE (0x0 << KVM_STATS_TYPE_SHIFT) > + #define KVM_STATS_TYPE_INSTANT (0x1 << KVM_STATS_TYPE_SHIFT) > + #define KVM_STATS_TYPE_MAX KVM_STATS_TYPE_INSTANT > + > + #define KVM_STATS_UNIT_SHIFT 4 > + #define KVM_STATS_UNIT_MASK (0xF << KVM_STATS_UNIT_SHIFT) > + #define KVM_STATS_UNIT_NONE (0x0 << KVM_STATS_UNIT_SHIFT) > + #define KVM_STATS_UNIT_BYTES (0x1 << KVM_STATS_UNIT_SHIFT) > + #define KVM_STATS_UNIT_SECONDS (0x2 << KVM_STATS_UNIT_SHIFT) > + #define KVM_STATS_UNIT_CYCLES (0x3 << KVM_STATS_UNIT_SHIFT) > + #define KVM_STATS_UNIT_MAX KVM_STATS_UNIT_CYCLES > + > + #define KVM_STATS_BASE_SHIFT 8 > + #define KVM_STATS_BASE_MASK (0xF << KVM_STATS_BASE_SHIFT) > + #define KVM_STATS_BASE_POW10 (0x0 << KVM_STATS_BASE_SHIFT) > + #define KVM_STATS_BASE_POW2 (0x1 << KVM_STATS_BASE_SHIFT) > + #define KVM_STATS_BASE_MAX KVM_STATS_BASE_POW2 > + > + struct kvm_stats_desc { > + __u32 flags; > + __s16 exponent; > + __u16 size; > + __u32 offset; > + __u32 unused; > + char name[]; > + }; As I mention in another patch, this should be sucked in directly from the .h file in kerneldoc format, so that everything stays in sync. I bet almost this whole file can be put into the .h file, look at how drm and v4l2 does this in a way that you only have to write the above one time, not try to keep it in sync in two different places. thanks, greg k-h
On 18/06/21 10:31, Greg KH wrote: > Ok, it's your maintenance burden, not mine, I was just suggesting a way > to make it easier :) > > I'll not complain about this anymore... I wish there was a way to keep them in sync without either sacrificing the quality of the documentation or reading kvm.h 100 times, I would jump on it! Paolo
On Fri, Jun 18, 2021 at 1:58 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Jun 18, 2021 at 04:48:15AM +0000, Jing Zhang wrote: > > Add a VM ioctl to get a statistics file descriptor by which a read > > functionality is provided for userspace to read out VM stats header, > > descriptors and data. > > Define VM statistics descriptors and header for all architectures. > > > > Reviewed-by: David Matlack <dmatlack@google.com> > > Reviewed-by: Ricardo Koller <ricarkol@google.com> > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > > Reviewed-by: Fuad Tabba <tabba@google.com> > > Tested-by: Fuad Tabba <tabba@google.com> #arm64 > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > arch/arm64/kvm/guest.c | 14 +++++++++++++ > > arch/mips/kvm/mips.c | 14 +++++++++++++ > > arch/powerpc/kvm/book3s.c | 16 +++++++++++++++ > > arch/powerpc/kvm/booke.c | 16 +++++++++++++++ > > arch/s390/kvm/kvm-s390.c | 19 +++++++++++++++++ > > arch/x86/kvm/x86.c | 24 ++++++++++++++++++++++ > > include/linux/kvm_host.h | 6 ++++++ > > virt/kvm/kvm_main.c | 43 +++++++++++++++++++++++++++++++++++++++ > > 8 files changed, 152 insertions(+) > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > index 4962331d01e6..f456d1defe2b 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c > > @@ -28,6 +28,20 @@ > > > > #include "trace.h" > > > > +struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > + KVM_GENERIC_VM_STATS() > > +}; > > +static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > > + sizeof(struct kvm_vm_stat) / sizeof(u64)); > > + > > +struct kvm_stats_header kvm_vm_stats_header = { > > Can this be const? > > > + .name_size = KVM_STATS_NAME_LEN, > > + .count = ARRAY_SIZE(kvm_vm_stats_desc), > > + .desc_offset = sizeof(struct kvm_stats_header) + KVM_STATS_ID_MAXLEN, > > + .data_offset = sizeof(struct kvm_stats_header) + KVM_STATS_ID_MAXLEN + > > + sizeof(kvm_vm_stats_desc), > > +}; > > If it can't be const, what is modified in it that prevents that from > happening? > > thanks, > > greg k-h Yes, it can be const. Thanks, Jing
This patchset provides a file descriptor for every VM and VCPU to read KVM statistics data in binary format. It is meant to provide a lightweight, flexible, scalable and efficient lock-free solution for user space telemetry applications to pull the statistics data periodically for large scale systems. The pulling frequency could be as high as a few times per second. In this patchset, every statistics data are treated to have some attributes as below: * architecture dependent or generic * VM statistics data or VCPU statistics data * type: cumulative, instantaneous, peak * unit: none for simple counter, nanosecond, microsecond, millisecond, second, Byte, KiByte, MiByte, GiByte, Clock Cycles Since no lock/synchronization is used, the consistency between all the statistics data is not guaranteed. That means not all statistics data are read out at the exact same time, since the statistics data are still being updated by KVM subsystems while they are read out. --- * v10 -> v11 - Rebase to kvm/queue, commit f1b832550832 (KVM: x86/mmu: Fix TDP MMU page table level) - Separate binary stats implementation commit - Use flexible length array member field in API structure instead of zero-length array member field - Move major binary stats reading function in a separate source file - Move stats id string into vm/vcpu structures - Add some detailed comments and update commit messages - Addressed some other review comments from Greg K.H. and Paolo. * v9 -> v10 - Relocate vcpu stat in vcpu's slab's usercopy region - Fix test issue for capability checking - Update commit message to explain why/how we need to add this new API for KVM statistics * v8 -> v9 - Rebase to commit 8331a2bc0898 (KVM: X86: Introduce KVM_HC_MAP_GPA_RANGE hypercall) - Reduce code duplication between binary and debugfs interface - Add field "offset" in stats descriptor to let us define stats descriptors in any order (not necessary in the order of stats defined in vm/vcpu stats structures) - Add static check to make sure the number of stats descriptors is the same as the number of stats defined in vm/vcpu stats structures - Fix missing/mismatched stats descriptor definition caused by rebase * v7 -> v8 - Rebase to kvm/queue, commit c1dc20e254b4 ("KVM: switch per-VM stats to u64") - Revise code to reflect the per-VM stats type from ulong to u64 - Addressed some other nits * v6 -> v7 - Improve file descriptor allocation function by Krish suggestion - Use "generic stats" instead of "common stats" as Krish suggested - Addressed some other nits from Krish and David Matlack * v5 -> v6 - Use designated initializers for STATS_DESC - Change KVM_STATS_SCALE... to KVM_STATS_BASE... - Use a common function for kvm_[vm|vcpu]_stats_read - Fix some documentation errors/missings - Use TEST_ASSERT in selftest - Use a common function for [vm|vcpu]_stats_test in selftest * v4 -> v5 - Rebase to kvm/queue, commit a4345a7cecfb ("Merge tag 'kvmarm-fixes-5.13-1'") - Change maximum stats name length to 48 - Replace VM_STATS_COMMON/VCPU_STATS_COMMON macros with stats descriptor definition macros. - Fixed some errors/warnings reported by checkpatch.pl * v3 -> v4 - Rebase to kvm/queue, commit 9f242010c3b4 ("KVM: avoid "deadlock" between install_new_memslots and MMU notifier") - Use C-stype comments in the whole patch - Fix wrong count for x86 VCPU stats descriptors - Fix KVM stats data size counting and validity check in selftest * v2 -> v3 - Rebase to kvm/queue, commit edf408f5257b ("KVM: avoid "deadlock" between install_new_memslots and MMU notifier") - Resolve some nitpicks about format * v1 -> v2 - Use ARRAY_SIZE to count the number of stats descriptors - Fix missing `size` field initialization in macro STATS_DESC [1] https://lore.kernel.org/kvm/20210402224359.2297157-1-jingzhangos@google.com [2] https://lore.kernel.org/kvm/20210415151741.1607806-1-jingzhangos@google.com [3] https://lore.kernel.org/kvm/20210423181727.596466-1-jingzhangos@google.com [4] https://lore.kernel.org/kvm/20210429203740.1935629-1-jingzhangos@google.com [5] https://lore.kernel.org/kvm/20210517145314.157626-1-jingzhangos@google.com [6] https://lore.kernel.org/kvm/20210524151828.4113777-1-jingzhangos@google.com [7] https://lore.kernel.org/kvm/20210603211426.790093-1-jingzhangos@google.com [8] https://lore.kernel.org/kvm/20210611124624.1404010-1-jingzhangos@google.com [9] https://lore.kernel.org/kvm/20210614212155.1670777-1-jingzhangos@google.com [10] https://lore.kernel.org/kvm/20210617044146.2667540-1-jingzhangos@google.com --- Jing Zhang (7): KVM: stats: Separate generic stats from architecture specific ones KVM: stats: Add fd-based API to read binary stats data KVM: stats: Support binary stats retrieval for a VM KVM: stats: Support binary stats retrieval for a VCPU KVM: stats: Add documentation for binary statistics interface KVM: selftests: Add selftest for KVM statistics data binary interface KVM: stats: Remove code duplication for binary and debugfs stats Documentation/virt/kvm/api.rst | 176 +++++++++++++- arch/arm64/include/asm/kvm_host.h | 9 +- arch/arm64/kvm/Makefile | 2 +- arch/arm64/kvm/guest.c | 46 ++-- arch/mips/include/asm/kvm_host.h | 9 +- arch/mips/kvm/Makefile | 2 +- arch/mips/kvm/mips.c | 88 ++++--- arch/powerpc/include/asm/kvm_host.h | 9 +- arch/powerpc/kvm/Makefile | 2 +- arch/powerpc/kvm/book3s.c | 89 ++++--- arch/powerpc/kvm/book3s_hv.c | 12 +- arch/powerpc/kvm/book3s_pr.c | 2 +- arch/powerpc/kvm/book3s_pr_papr.c | 2 +- arch/powerpc/kvm/booke.c | 74 ++++-- arch/s390/include/asm/kvm_host.h | 9 +- arch/s390/kvm/Makefile | 3 +- arch/s390/kvm/kvm-s390.c | 230 ++++++++++-------- arch/x86/include/asm/kvm_host.h | 9 +- arch/x86/kvm/Makefile | 2 +- arch/x86/kvm/x86.c | 107 ++++---- include/linux/kvm_host.h | 182 ++++++++++++-- include/linux/kvm_types.h | 12 + include/uapi/linux/kvm.h | 44 ++++ tools/testing/selftests/kvm/.gitignore | 1 + tools/testing/selftests/kvm/Makefile | 3 + .../testing/selftests/kvm/include/kvm_util.h | 3 + .../selftests/kvm/kvm_binary_stats_test.c | 225 +++++++++++++++++ tools/testing/selftests/kvm/lib/kvm_util.c | 12 + virt/kvm/binary_stats.c | 130 ++++++++++ virt/kvm/kvm_main.c | 218 ++++++++++++++--- 30 files changed, 1355 insertions(+), 357 deletions(-) create mode 100644 tools/testing/selftests/kvm/kvm_binary_stats_test.c create mode 100644 virt/kvm/binary_stats.c base-commit: f1b8325508327a302f1d5cd8a4bf51e2c9c72fa9