| Message ID | 20210617044146.2667540-4-jingzhangos@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM statistics data fd-based binary interface | expand |
On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > + struct kvm_stats_desc { > + __u32 flags; > + __s16 exponent; > + __u16 size; > + __u32 offset; > + __u32 unused; > + char name[0]; > + }; > + > +The ``flags`` field contains the type and unit of the statistics data described > +by this descriptor. The following flags are supported: > + > +Bits 0-3 of ``flags`` encode the type: <snip> As flags is a bit field, what is the endian of it? Native or LE or BE? I'm guessing "cpu native", but you should be explicit as userspace will have to deal with this somehow. thanks, greg k-h
On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > + struct kvm_stats_desc { > + __u32 flags; > + __s16 exponent; > + __u16 size; > + __u32 offset; > + __u32 unused; > + char name[0]; > + }; <snip> > +The ``unused`` fields are reserved for future support for other types of > +statistics data, like log/linear histogram. you HAVE to set unused to 0 for now, otherwise userspace does not know it is unused, right? And then, really it is "used", so why not just say that now? It's tricky, but you have to get this right now otherwise you can never use it in the future. > +The ``name`` field points to the name string of the statistics data. The name It is not a pointer, it is the data itself. > +string starts at the end of ``struct kvm_stats_desc``. > +The maximum length (including trailing '\0') is indicated by ``name_size`` > +in ``struct kvm_stats_header``. I thought we were replacing [0] arrays with [], are you sure you should be declaring this as [0]? Same for all structures in this document (and code). thanks, greg k-h
On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > +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[0]; > + }; So you have no idea the size of the whole header when reading it? That feels odd, are you sure it's not needed? > +The ``id`` field is identification for 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". Why do you have "name_size" but not "id_size"? And is this a \0 terminated string? If so, please state it here. And what is the max size of this string? And again, should it be [], not [0]? Will the header be padded out to any specific byte boundry (4/8/32/whatever) before the other headers? > + > +The ``name_size`` field is the size (byte) of the statistics name string s/byte/in bytes/ > +(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[0]; > + }; > + > +The ``flags`` field contains the type and unit of the statistics data described > +by this descriptor. The following flags are supported: > + > +Bits 0-3 of ``flags`` encode the type: > + * ``KVM_STATS_TYPE_CUMULATIVE`` > + The statistics data is cumulative. The value of data can only be increased. > + Most of the counters used in KVM are of this type. > + The corresponding ``count`` field for this type is always 1. > + * ``KVM_STATS_TYPE_INSTANT`` > + The statistics data is instantaneous. Its value can be increased or > + decreased. This type is usually used as a measurement of some resources, > + like the number of dirty pages, the number of large pages, etc. > + The corresponding ``count`` field for this type is always 1. > + > +Bits 4-7 of ``flags`` encode the unit: > + * ``KVM_STATS_UNIT_NONE`` > + There is no unit for the value of statistics data. This usually means that > + the value is a simple counter of an event. > + * ``KVM_STATS_UNIT_BYTES`` > + It indicates that the statistics data is used to measure memory size, in the > + unit of Byte, KiByte, MiByte, GiByte, etc. The unit of the data is > + determined by the ``exponent`` field in the descriptor. The > + ``KVM_STATS_BASE_POW2`` flag is valid in this case. The unit of the data is > + determined by ``pow(2, exponent)``. For example, if value is 10, > + ``exponent`` is 20, which means the unit of statistics data is MiByte, we > + can get the statistics data in the unit of Byte by > + ``value * pow(2, exponent) = 10 * pow(2, 20) = 10 MiByte`` which is > + 10 * 1024 * 1024 Bytes. > + * ``KVM_STATS_UNIT_SECONDS`` > + It indicates that the statistics data is used to measure time/latency, in > + the unit of nanosecond, microsecond, millisecond and second. The unit of the > + data is determined by the ``exponent`` field in the descriptor. The > + ``KVM_STATS_BASE_POW10`` flag is valid in this case. The unit of the data > + is determined by ``pow(10, exponent)``. For example, if value is 2000000, > + ``exponent`` is -6, which means the unit of statistics data is microsecond, > + we can get the statistics data in the unit of second by > + ``value * pow(10, exponent) = 2000000 * pow(10, -6) = 2 seconds``. > + * ``KVM_STATS_UNIT_CYCLES`` > + It indicates that the statistics data is used to measure CPU clock cycles. > + The ``KVM_STATS_BASE_POW10`` flag is valid in this case. For example, if > + value is 200, ``exponent`` is 4, we can get the number of CPU clock cycles > + by ``value * pow(10, exponent) = 200 * pow(10, 4) = 2000000``. > + > +Bits 8-11 of ``flags`` encode the base: > + * ``KVM_STATS_BASE_POW10`` > + The scale is based on power of 10. It is used for measurement of time and > + CPU clock cycles. > + * ``KVM_STATS_BASE_POW2`` > + The scale is based on power of 2. It is used for measurement of memory size. > + > +The ``exponent`` field is the scale of corresponding statistics data. For > +example, if the unit is ``KVM_STATS_UNIT_BYTES``, the base is > +``KVM_STATS_BASE_POW2``, the ``exponent`` is 10, then we know that the real > +unit of the statistics data is KBytes a.k.a pow(2, 10) = 1024 bytes. Might also want to show a negative example here for exponent, like you show above for time. > + > +The ``size`` field is the number of values (u64) of this statistics data. Its > +value is usually 1 for most of simple statistics. What does "u64" mean here? thanks, greg k-h
On Thu, Jun 17, 2021 at 04:41:44AM +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. Do you have a pointer to userspace code that can do such a thing that others can use? We do not like adding apis to the kernel without at least seeing the user of those apis, especially for complex things like this. Ideally you would include some library code in the kernel tree itself that everyone can use for this for their own programs. You have provided a test which is great, but how do we know it works for "real" usages? thanks, greg k-h
On 17/06/21 08:07, Greg KH wrote: >> The statistics data itself could be read out by userspace telemetry >> periodically without any extra parsing or setup effort. > Do you have a pointer to userspace code that can do such a thing that > others can use? We do not like adding apis to the kernel without at > least seeing the user of those apis, especially for complex things like > this. > > Ideally you would include some library code in the kernel tree itself > that everyone can use for this for their own programs. You have > provided a test which is great, but how do we know it works for "real" > usages? I am pretty sure that Google is using this internally, but we are also going to work on QEMU and Libvirt support for this. As for the rest, thanks for the review---I'll let Jing act on it and only add my own remarks in a couple places. Paolo
On 17/06/21 07:56, Greg KH wrote: > On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: >> + struct kvm_stats_desc { >> + __u32 flags; >> + __s16 exponent; >> + __u16 size; >> + __u32 offset; >> + __u32 unused; >> + char name[0]; >> + }; > > <snip> > >> +The ``unused`` fields are reserved for future support for other types of >> +statistics data, like log/linear histogram. > > you HAVE to set unused to 0 for now, otherwise userspace does not know > it is unused, right? Jing, I think you planned to use it with other flags that are unused for now? But please do check that it's zero in the testcase. > It is not a pointer, it is the data itself. > >> +string starts at the end of ``struct kvm_stats_desc``. >> +The maximum length (including trailing '\0') is indicated by ``name_size`` >> +in ``struct kvm_stats_header``. > > I thought we were replacing [0] arrays with [], are you sure you should > be declaring this as [0]? Same for all structures in this document (and > code). In C code [0] is a bit more flexible than []. I think in this particular case [] won't work due to how the structures are declared. In the documentation [] is certainly clearer. Paolo
On Thu, Jun 17, 2021 at 01:19:50PM +0200, Paolo Bonzini wrote: > On 17/06/21 08:07, Greg KH wrote: > > > The statistics data itself could be read out by userspace telemetry > > > periodically without any extra parsing or setup effort. > > Do you have a pointer to userspace code that can do such a thing that > > others can use? We do not like adding apis to the kernel without at > > least seeing the user of those apis, especially for complex things like > > this. > > > > Ideally you would include some library code in the kernel tree itself > > that everyone can use for this for their own programs. You have > > provided a test which is great, but how do we know it works for "real" > > usages? > > I am pretty sure that Google is using this internally, but we are also going > to work on QEMU and Libvirt support for this. We need an "external user" for something as complex as this to be able to see if it actually works or not. Otherwise we have to just guess :( thanks, greg k-h
On Thu, Jun 17, 2021 at 01:29:22PM +0200, Paolo Bonzini wrote: > On 17/06/21 07:56, Greg KH wrote: > > On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > > > + struct kvm_stats_desc { > > > + __u32 flags; > > > + __s16 exponent; > > > + __u16 size; > > > + __u32 offset; > > > + __u32 unused; > > > + char name[0]; > > > + }; > > > > <snip> > > > > > +The ``unused`` fields are reserved for future support for other types of > > > +statistics data, like log/linear histogram. > > > > you HAVE to set unused to 0 for now, otherwise userspace does not know > > it is unused, right? > > Jing, I think you planned to use it with other flags that are unused for > now? But please do check that it's zero in the testcase. > > > It is not a pointer, it is the data itself. > > > > > +string starts at the end of ``struct kvm_stats_desc``. > > > +The maximum length (including trailing '\0') is indicated by ``name_size`` > > > +in ``struct kvm_stats_header``. > > > > I thought we were replacing [0] arrays with [], are you sure you should > > be declaring this as [0]? Same for all structures in this document (and > > code). > > In C code [0] is a bit more flexible than []. I think in this particular > case [] won't work due to how the structures are declared. In the > documentation [] is certainly clearer. Look at all of the patches that Gustavo has been doing all over the tree for this work, you do not want to make him do this again here. Gustavo, is [0] ok for fields like these? thanks, greg k-h
On 17/06/21 13:42, Greg KH wrote: > On Thu, Jun 17, 2021 at 01:29:22PM +0200, Paolo Bonzini wrote: >> On 17/06/21 07:56, Greg KH wrote: >>> On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: >>>> +string starts at the end of ``struct kvm_stats_desc``. >>>> +The maximum length (including trailing '\0') is indicated by ``name_size`` >>>> +in ``struct kvm_stats_header``. >>> >>> I thought we were replacing [0] arrays with [], are you sure you should >>> be declaring this as [0]? Same for all structures in this document (and >>> code). >> >> In C code [0] is a bit more flexible than []. I think in this particular >> case [] won't work due to how the structures are declared. In the >> documentation [] is certainly clearer. > > Look at all of the patches that Gustavo has been doing all over the tree > for this work, you do not want to make him do this again here. > > Gustavo, is [0] ok for fields like these? I should be able to get back to KVM stuff later today, I'll check myself if [] can be applied and reply. I had queued an early version of these for my local build to play with them but I haven't been able to do a complete review. Paolo
Hi Greg, On Thu, Jun 17, 2021 at 12:52 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > > + struct kvm_stats_desc { > > + __u32 flags; > > + __s16 exponent; > > + __u16 size; > > + __u32 offset; > > + __u32 unused; > > + char name[0]; > > + }; > > + > > +The ``flags`` field contains the type and unit of the statistics data described > > +by this descriptor. The following flags are supported: > > + > > +Bits 0-3 of ``flags`` encode the type: > > <snip> > > As flags is a bit field, what is the endian of it? Native or LE or BE? > I'm guessing "cpu native", but you should be explicit as userspace will > have to deal with this somehow. Sure, will add clarification for endianness. > > thanks, > > greg k-h Thanks, Jing
Hi Greg, On Thu, Jun 17, 2021 at 12:56 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > > + struct kvm_stats_desc { > > + __u32 flags; > > + __s16 exponent; > > + __u16 size; > > + __u32 offset; > > + __u32 unused; > > + char name[0]; > > + }; > > <snip> > > > +The ``unused`` fields are reserved for future support for other types of > > +statistics data, like log/linear histogram. > > you HAVE to set unused to 0 for now, otherwise userspace does not know > it is unused, right? And then, really it is "used", so why not just say > that now? It's tricky, but you have to get this right now otherwise you > can never use it in the future. > Sure, will do that. > > +The ``name`` field points to the name string of the statistics data. The name > > It is not a pointer, it is the data itself. > Will fix it. > > +string starts at the end of ``struct kvm_stats_desc``. > > +The maximum length (including trailing '\0') is indicated by ``name_size`` > > +in ``struct kvm_stats_header``. > > I thought we were replacing [0] arrays with [], are you sure you should > be declaring this as [0]? Same for all structures in this document (and > code). > The reason to declare it as [0] is to have the flexibility to change the maximum length of KVM stats name. For now, the max len is defined as 48, which can be read from the header. Then the userspace can get the length of descriptor by adding sizeof(struct_kvm_stats_desc) + 48. Whenever the max len is changed in KVM, the userspace would not have to update code to reflect that. However, if we are OK to restrict the maximum KVM stats' length to 48 (or any other number), we can just declear it with [] instead of [0]. > thanks, > > greg k-h Thanks, Jing
Hi Greg, On Thu, Jun 17, 2021 at 1:05 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > > +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[0]; > > + }; > > So you have no idea the size of the whole header when reading it? That > feels odd, are you sure it's not needed? > We do know the size when reading it, it is sizeof(struct kvm_stats_header) + KVM_STATS_ID_MAXLEN. I think we need to change "char id[0]" to "char id[KVM_STATS_ID_MAXLEN]" here. Since it is not like the name field in the descriptor, the max id string's length is fixed. > > +The ``id`` field is identification for 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". > > Why do you have "name_size" but not "id_size"? Because we know that the max length of id is less than 64, that's why we have KVM_STATS_ID_MAXLEN exported to userspace. But for KVM stats name, we are not sure about the max length in the future. The maximum length for KVM stats is defined as 48 in KVM for now, but it is possible that it might need to be increased to 64 or larger. > > And is this a \0 terminated string? If so, please state it here. > Will do. > And what is the max size of this string? > > And again, should it be [], not [0]? As stated in previous comments, there are pros and cons to choose between [] and [0]. > > Will the header be padded out to any specific byte boundry > (4/8/32/whatever) before the other headers? > No. For every architecture, there is only one header for VM and one header for VCPU. > > + > > +The ``name_size`` field is the size (byte) of the statistics name string > > s/byte/in bytes/ > Will do. > > +(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[0]; > > + }; > > + > > +The ``flags`` field contains the type and unit of the statistics data described > > +by this descriptor. The following flags are supported: > > + > > +Bits 0-3 of ``flags`` encode the type: > > + * ``KVM_STATS_TYPE_CUMULATIVE`` > > + The statistics data is cumulative. The value of data can only be increased. > > + Most of the counters used in KVM are of this type. > > + The corresponding ``count`` field for this type is always 1. > > + * ``KVM_STATS_TYPE_INSTANT`` > > + The statistics data is instantaneous. Its value can be increased or > > + decreased. This type is usually used as a measurement of some resources, > > + like the number of dirty pages, the number of large pages, etc. > > + The corresponding ``count`` field for this type is always 1. > > + > > +Bits 4-7 of ``flags`` encode the unit: > > + * ``KVM_STATS_UNIT_NONE`` > > + There is no unit for the value of statistics data. This usually means that > > + the value is a simple counter of an event. > > + * ``KVM_STATS_UNIT_BYTES`` > > + It indicates that the statistics data is used to measure memory size, in the > > + unit of Byte, KiByte, MiByte, GiByte, etc. The unit of the data is > > + determined by the ``exponent`` field in the descriptor. The > > + ``KVM_STATS_BASE_POW2`` flag is valid in this case. The unit of the data is > > + determined by ``pow(2, exponent)``. For example, if value is 10, > > + ``exponent`` is 20, which means the unit of statistics data is MiByte, we > > + can get the statistics data in the unit of Byte by > > + ``value * pow(2, exponent) = 10 * pow(2, 20) = 10 MiByte`` which is > > + 10 * 1024 * 1024 Bytes. > > + * ``KVM_STATS_UNIT_SECONDS`` > > + It indicates that the statistics data is used to measure time/latency, in > > + the unit of nanosecond, microsecond, millisecond and second. The unit of the > > + data is determined by the ``exponent`` field in the descriptor. The > > + ``KVM_STATS_BASE_POW10`` flag is valid in this case. The unit of the data > > + is determined by ``pow(10, exponent)``. For example, if value is 2000000, > > + ``exponent`` is -6, which means the unit of statistics data is microsecond, > > + we can get the statistics data in the unit of second by > > + ``value * pow(10, exponent) = 2000000 * pow(10, -6) = 2 seconds``. > > + * ``KVM_STATS_UNIT_CYCLES`` > > + It indicates that the statistics data is used to measure CPU clock cycles. > > + The ``KVM_STATS_BASE_POW10`` flag is valid in this case. For example, if > > + value is 200, ``exponent`` is 4, we can get the number of CPU clock cycles > > + by ``value * pow(10, exponent) = 200 * pow(10, 4) = 2000000``. > > + > > +Bits 8-11 of ``flags`` encode the base: > > + * ``KVM_STATS_BASE_POW10`` > > + The scale is based on power of 10. It is used for measurement of time and > > + CPU clock cycles. > > + * ``KVM_STATS_BASE_POW2`` > > + The scale is based on power of 2. It is used for measurement of memory size. > > + > > +The ``exponent`` field is the scale of corresponding statistics data. For > > +example, if the unit is ``KVM_STATS_UNIT_BYTES``, the base is > > +``KVM_STATS_BASE_POW2``, the ``exponent`` is 10, then we know that the real > > +unit of the statistics data is KBytes a.k.a pow(2, 10) = 1024 bytes. > > Might also want to show a negative example here for exponent, like you > show above for time. > > Will do. > > + > > +The ``size`` field is the number of values (u64) of this statistics data. Its > > +value is usually 1 for most of simple statistics. > > What does "u64" mean here? It means the type of value is unsigned 64 bit. Will clarify this. > > thanks, > > greg k-h Thanks, Jing
On Thu, Jun 17, 2021 at 6:32 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 17/06/21 07:56, Greg KH wrote: > > On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > >> + struct kvm_stats_desc { > >> + __u32 flags; > >> + __s16 exponent; > >> + __u16 size; > >> + __u32 offset; > >> + __u32 unused; > >> + char name[0]; > >> + }; > > > > <snip> > > > >> +The ``unused`` fields are reserved for future support for other types of > >> +statistics data, like log/linear histogram. > > > > you HAVE to set unused to 0 for now, otherwise userspace does not know > > it is unused, right? > > Jing, I think you planned to use it with other flags that are unused for > now? But please do check that it's zero in the testcase. > Yes, it was planned for future use (to support stats type of histogram). Will add check in testcase and clarify it in doc. > > It is not a pointer, it is the data itself. > > > >> +string starts at the end of ``struct kvm_stats_desc``. > >> +The maximum length (including trailing '\0') is indicated by ``name_size`` > >> +in ``struct kvm_stats_header``. > > > > I thought we were replacing [0] arrays with [], are you sure you should > > be declaring this as [0]? Same for all structures in this document (and > > code). > > In C code [0] is a bit more flexible than []. I think in this > particular case [] won't work due to how the structures are declared. > In the documentation [] is certainly clearer. > > Paolo > Thanks, Jing
On Thu, Jun 17, 2021 at 6:34 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 17, 2021 at 01:19:50PM +0200, Paolo Bonzini wrote: > > On 17/06/21 08:07, Greg KH wrote: > > > > The statistics data itself could be read out by userspace telemetry > > > > periodically without any extra parsing or setup effort. > > > Do you have a pointer to userspace code that can do such a thing that > > > others can use? We do not like adding apis to the kernel without at > > > least seeing the user of those apis, especially for complex things like > > > this. > > > > > > Ideally you would include some library code in the kernel tree itself > > > that everyone can use for this for their own programs. You have > > > provided a test which is great, but how do we know it works for "real" > > > usages? > > > > I am pretty sure that Google is using this internally, but we are also going > > to work on QEMU and Libvirt support for this. > > We need an "external user" for something as complex as this to be able > to see if it actually works or not. Otherwise we have to just guess :( We have plans to add some library code in kernel selftests and also some simple tools to let everyone get a feeling of the new API. > > thanks, > > greg k-h Thanks, Jing
Hi Greg, On Thu, Jun 17, 2021 at 10:20 AM Jing Zhang <jingzhangos@google.com> wrote: > > Hi Greg, > > On Thu, Jun 17, 2021 at 12:56 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Jun 17, 2021 at 04:41:44AM +0000, Jing Zhang wrote: > > > + struct kvm_stats_desc { > > > + __u32 flags; > > > + __s16 exponent; > > > + __u16 size; > > > + __u32 offset; > > > + __u32 unused; > > > + char name[0]; > > > + }; > > > > <snip> > > > > > +The ``unused`` fields are reserved for future support for other types of > > > +statistics data, like log/linear histogram. > > > > you HAVE to set unused to 0 for now, otherwise userspace does not know > > it is unused, right? And then, really it is "used", so why not just say > > that now? It's tricky, but you have to get this right now otherwise you > > can never use it in the future. > > > Sure, will do that. > > > +The ``name`` field points to the name string of the statistics data. The name > > > > It is not a pointer, it is the data itself. > > > Will fix it. > > > +string starts at the end of ``struct kvm_stats_desc``. > > > +The maximum length (including trailing '\0') is indicated by ``name_size`` > > > +in ``struct kvm_stats_header``. > > > > I thought we were replacing [0] arrays with [], are you sure you should > > be declaring this as [0]? Same for all structures in this document (and > > code). > > > The reason to declare it as [0] is to have the flexibility to change the maximum > length of KVM stats name. For now, the max len is defined as 48, which can > be read from the header. Then the userspace can get the length of descriptor by > adding sizeof(struct_kvm_stats_desc) + 48. Whenever the max len is changed > in KVM, the userspace would not have to update code to reflect that. > However, if we are OK to restrict the maximum KVM stats' length to 48 > (or any other > number), we can just declear it with [] instead of [0]. Please ignore my above comments. You are right. We can just replace all zero-length arrays [0] with a flexible array member []. Thanks. > > thanks, > > > > greg k-h > > Thanks, > Jing
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index e328caa35d6c..35ee52dbec89 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,174 @@ 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[0]; + }; + +The ``id`` field is identification for 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 (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[0]; + }; + +The ``flags`` field contains the type and unit of the statistics data described +by this descriptor. The following flags are supported: + +Bits 0-3 of ``flags`` encode the type: + * ``KVM_STATS_TYPE_CUMULATIVE`` + The statistics data is cumulative. The value of data can only be increased. + Most of the counters used in KVM are of this type. + The corresponding ``count`` field for this type is always 1. + * ``KVM_STATS_TYPE_INSTANT`` + The statistics data is instantaneous. Its value can be increased or + decreased. This type is usually used as a measurement of some resources, + like the number of dirty pages, the number of large pages, etc. + The corresponding ``count`` field for this type is always 1. + +Bits 4-7 of ``flags`` encode the unit: + * ``KVM_STATS_UNIT_NONE`` + There is no unit for the value of statistics data. This usually means that + the value is a simple counter of an event. + * ``KVM_STATS_UNIT_BYTES`` + It indicates that the statistics data is used to measure memory size, in the + unit of Byte, KiByte, MiByte, GiByte, etc. The unit of the data is + determined by the ``exponent`` field in the descriptor. The + ``KVM_STATS_BASE_POW2`` flag is valid in this case. The unit of the data is + determined by ``pow(2, exponent)``. For example, if value is 10, + ``exponent`` is 20, which means the unit of statistics data is MiByte, we + can get the statistics data in the unit of Byte by + ``value * pow(2, exponent) = 10 * pow(2, 20) = 10 MiByte`` which is + 10 * 1024 * 1024 Bytes. + * ``KVM_STATS_UNIT_SECONDS`` + It indicates that the statistics data is used to measure time/latency, in + the unit of nanosecond, microsecond, millisecond and second. The unit of the + data is determined by the ``exponent`` field in the descriptor. The + ``KVM_STATS_BASE_POW10`` flag is valid in this case. The unit of the data + is determined by ``pow(10, exponent)``. For example, if value is 2000000, + ``exponent`` is -6, which means the unit of statistics data is microsecond, + we can get the statistics data in the unit of second by + ``value * pow(10, exponent) = 2000000 * pow(10, -6) = 2 seconds``. + * ``KVM_STATS_UNIT_CYCLES`` + It indicates that the statistics data is used to measure CPU clock cycles. + The ``KVM_STATS_BASE_POW10`` flag is valid in this case. For example, if + value is 200, ``exponent`` is 4, we can get the number of CPU clock cycles + by ``value * pow(10, exponent) = 200 * pow(10, 4) = 2000000``. + +Bits 8-11 of ``flags`` encode the base: + * ``KVM_STATS_BASE_POW10`` + The scale is based on power of 10. It is used for measurement of time and + CPU clock cycles. + * ``KVM_STATS_BASE_POW2`` + The scale is based on power of 2. It is used for measurement of memory size. + +The ``exponent`` field is the scale of corresponding statistics data. For +example, if the unit is ``KVM_STATS_UNIT_BYTES``, the base is +``KVM_STATS_BASE_POW2``, the ``exponent`` is 10, then we know that the real +unit of the statistics data is KBytes a.k.a pow(2, 10) = 1024 bytes. + +The ``size`` field is the number of values (u64) of this statistics data. Its +value is usually 1 for most of simple statistics. + +The ``offset`` field is the offset from the start of Data Block to the start of +the corresponding statistics data. + +The ``unused`` fields are reserved for future support for other types of +statistics data, like log/linear histogram. + +The ``name`` field points to the name string of the statistics data. The name +string starts at the end of ``struct kvm_stats_desc``. +The maximum length (including trailing '\0') is indicated by ``name_size`` +in ``struct kvm_stats_header``. + +The Stats Data block contains an array of data values of type ``struct +kvm_vm_stats_data`` or ``struct kvm_vcpu_stats_data``. It would be read by +userspace periodically to pull statistics data. +The order of data value in Stats Data block is the same as the order of +descriptors in Descriptors block. + * Statistics data for VM/VCPU:: + + struct kvm_stats_data { + __u64 value[0]; + }; 5. The kvm_run structure ======================== @@ -6969,3 +7136,11 @@ The argument to KVM_ENABLE_CAP is also a bitmask, and must be a subset of the result of KVM_CHECK_EXTENSION. KVM will forward to userspace the hypercalls whose corresponding bit is in the argument, and return ENOSYS for the others. + +8.35 KVM_CAP_STATS_BINARY_FD +---------------------------- + +:Architectures: all + +This capability indicates the feature that userspace can create get a file +descriptor for every VM and VCPU to read statistics data in binary format.