mbox series

[v7,0/4] KVM statistics data fd-based binary interface

Message ID 20210603211426.790093-1-jingzhangos@google.com
Headers show
Series KVM statistics data fd-based binary interface | expand

Message

Jing Zhang June 3, 2021, 9:14 p.m. UTC
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,
  * 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 date
are still being updated by KVM subsystems while they are read out.

---

* 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

---

Jing Zhang (4):
  KVM: stats: Separate generic stats from architecture specific ones
  KVM: stats: Add fd-based API to read binary stats data
  KVM: stats: Add documentation for statistics data binary interface
  KVM: selftests: Add selftest for KVM statistics data binary interface

 Documentation/virt/kvm/api.rst                | 180 +++++++++++++++
 arch/arm64/include/asm/kvm_host.h             |   9 +-
 arch/arm64/kvm/guest.c                        |  38 +++-
 arch/mips/include/asm/kvm_host.h              |   9 +-
 arch/mips/kvm/mips.c                          |  64 +++++-
 arch/powerpc/include/asm/kvm_host.h           |   9 +-
 arch/powerpc/kvm/book3s.c                     |  64 +++++-
 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                      |  59 ++++-
 arch/s390/include/asm/kvm_host.h              |   9 +-
 arch/s390/kvm/kvm-s390.c                      | 129 ++++++++++-
 arch/x86/include/asm/kvm_host.h               |   9 +-
 arch/x86/kvm/x86.c                            |  67 +++++-
 include/linux/kvm_host.h                      | 141 +++++++++++-
 include/linux/kvm_types.h                     |  12 +
 include/uapi/linux/kvm.h                      |  50 ++++
 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     | 215 ++++++++++++++++++
 tools/testing/selftests/kvm/lib/kvm_util.c    |  12 +
 virt/kvm/kvm_main.c                           | 169 +++++++++++++-
 24 files changed, 1178 insertions(+), 90 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/kvm_binary_stats_test.c


base-commit: a4345a7cecfb91ae78cd43d26b0c6a956420761a

Comments

Paolo Bonzini June 10, 2021, 4:46 p.m. UTC | #1
On 03/06/21 23:14, Jing Zhang wrote:
> 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,

>    * 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 date

> are still being updated by KVM subsystems while they are read out.

> 

> ---

> 

> * 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

> 

> ---

> 

> Jing Zhang (4):

>    KVM: stats: Separate generic stats from architecture specific ones

>    KVM: stats: Add fd-based API to read binary stats data

>    KVM: stats: Add documentation for statistics data binary interface

>    KVM: selftests: Add selftest for KVM statistics data binary interface

> 

>   Documentation/virt/kvm/api.rst                | 180 +++++++++++++++

>   arch/arm64/include/asm/kvm_host.h             |   9 +-

>   arch/arm64/kvm/guest.c                        |  38 +++-

>   arch/mips/include/asm/kvm_host.h              |   9 +-

>   arch/mips/kvm/mips.c                          |  64 +++++-

>   arch/powerpc/include/asm/kvm_host.h           |   9 +-

>   arch/powerpc/kvm/book3s.c                     |  64 +++++-

>   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                      |  59 ++++-

>   arch/s390/include/asm/kvm_host.h              |   9 +-

>   arch/s390/kvm/kvm-s390.c                      | 129 ++++++++++-

>   arch/x86/include/asm/kvm_host.h               |   9 +-

>   arch/x86/kvm/x86.c                            |  67 +++++-

>   include/linux/kvm_host.h                      | 141 +++++++++++-

>   include/linux/kvm_types.h                     |  12 +

>   include/uapi/linux/kvm.h                      |  50 ++++

>   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     | 215 ++++++++++++++++++

>   tools/testing/selftests/kvm/lib/kvm_util.c    |  12 +

>   virt/kvm/kvm_main.c                           | 169 +++++++++++++-

>   24 files changed, 1178 insertions(+), 90 deletions(-)

>   create mode 100644 tools/testing/selftests/kvm/kvm_binary_stats_test.c

> 

> 

> base-commit: a4345a7cecfb91ae78cd43d26b0c6a956420761a

> 


I had a few remarks, but it looks very nice overall.

Thanks!

Paolo
Jing Zhang June 10, 2021, 5:30 p.m. UTC | #2
Hi Paolo,

On Thu, Jun 10, 2021 at 11:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>

> On 03/06/21 23:14, Jing Zhang wrote:

> > 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,

> >    * 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 date

> > are still being updated by KVM subsystems while they are read out.

> >

> > ---

> >

> > * 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

> >

> > ---

> >

> > Jing Zhang (4):

> >    KVM: stats: Separate generic stats from architecture specific ones

> >    KVM: stats: Add fd-based API to read binary stats data

> >    KVM: stats: Add documentation for statistics data binary interface

> >    KVM: selftests: Add selftest for KVM statistics data binary interface

> >

> >   Documentation/virt/kvm/api.rst                | 180 +++++++++++++++

> >   arch/arm64/include/asm/kvm_host.h             |   9 +-

> >   arch/arm64/kvm/guest.c                        |  38 +++-

> >   arch/mips/include/asm/kvm_host.h              |   9 +-

> >   arch/mips/kvm/mips.c                          |  64 +++++-

> >   arch/powerpc/include/asm/kvm_host.h           |   9 +-

> >   arch/powerpc/kvm/book3s.c                     |  64 +++++-

> >   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                      |  59 ++++-

> >   arch/s390/include/asm/kvm_host.h              |   9 +-

> >   arch/s390/kvm/kvm-s390.c                      | 129 ++++++++++-

> >   arch/x86/include/asm/kvm_host.h               |   9 +-

> >   arch/x86/kvm/x86.c                            |  67 +++++-

> >   include/linux/kvm_host.h                      | 141 +++++++++++-

> >   include/linux/kvm_types.h                     |  12 +

> >   include/uapi/linux/kvm.h                      |  50 ++++

> >   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     | 215 ++++++++++++++++++

> >   tools/testing/selftests/kvm/lib/kvm_util.c    |  12 +

> >   virt/kvm/kvm_main.c                           | 169 +++++++++++++-

> >   24 files changed, 1178 insertions(+), 90 deletions(-)

> >   create mode 100644 tools/testing/selftests/kvm/kvm_binary_stats_test.c

> >

> >

> > base-commit: a4345a7cecfb91ae78cd43d26b0c6a956420761a

> >

>

> I had a few remarks, but it looks very nice overall.

>

> Thanks!

>

> Paolo

>

Happy to see per-VM stats use u64 too.
Will send out another patchset based on your remarks.

Thanks,
Jing
Christian Borntraeger June 11, 2021, 6:57 a.m. UTC | #3
On 03.06.21 23:14, Jing Zhang wrote:
> 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


Are the debugfs things good enough, or do we want to also add the same
ioctl for the /dev/kvm to get the global counters as well, e.g. for
tools like kvm_stat?
Paolo Bonzini June 11, 2021, 11:02 a.m. UTC | #4
On 11/06/21 08:57, Christian Borntraeger wrote:
> 

> On 03.06.21 23:14, Jing Zhang wrote:

>> 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

> 

> Are the debugfs things good enough, or do we want to also add the same

> ioctl for the /dev/kvm to get the global counters as well, e.g. for

> tools like kvm_stat?


I think we should, yes.  We should also add the summarized VCPU 
statistics to the VM-level file descriptor.  However, it can be done in 
steps.

Paolo