| Message ID | 20220411211015.3091615-5-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Add a cap to disable NX hugepages on a VM | expand |
On Mon, Apr 11, 2022 at 2:10 PM Ben Gardon <bgardon@google.com> wrote: > > Move the code to read the binary stats data to the KVM selftests > library. It will be re-used by other tests to check KVM behavior. > > No functional change intended. > > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/include/kvm_util_base.h | 3 +++ > .../selftests/kvm/kvm_binary_stats_test.c | 20 +++++------------- > tools/testing/selftests/kvm/lib/kvm_util.c | 21 +++++++++++++++++++ > 3 files changed, 29 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index c5f34551ff76..b2684cfc2cb1 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -405,6 +405,9 @@ struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > struct kvm_stats_header *header); > void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > struct kvm_stats_desc *stats_desc); > +int read_stat_data(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *desc, uint64_t *data, > + ssize_t max_elements); > > uint32_t guest_get_vcpuid(void); > > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > index e4795bad7db6..97b180249ba0 100644 > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > @@ -20,6 +20,8 @@ > #include "asm/kvm.h" > #include "linux/kvm.h" > > +#define STAT_MAX_ELEMENTS 1000 > + > static void stats_test(int stats_fd) > { > ssize_t ret; > @@ -29,7 +31,7 @@ static void stats_test(int stats_fd) > struct kvm_stats_header header; > char *id; > struct kvm_stats_desc *stats_desc; > - u64 *stats_data; > + u64 stats_data[STAT_MAX_ELEMENTS]; What is the benefit of changing stats_data to a stack allocation with a fixed limit? > struct kvm_stats_desc *pdesc; > > /* Read kvm stats header */ > @@ -130,25 +132,13 @@ static void stats_test(int stats_fd) > pdesc->offset, pdesc->name); > } > > - /* Allocate memory for stats data */ > - stats_data = malloc(size_data); > - TEST_ASSERT(stats_data, "Allocate memory for stats data"); > - /* Read kvm stats data as a bulk */ > - ret = pread(stats_fd, stats_data, size_data, header.data_offset); > - TEST_ASSERT(ret == size_data, "Read KVM stats data"); > /* Read kvm stats data one by one */ > - size_data = 0; > for (i = 0; i < header.num_desc; ++i) { > pdesc = (void *)stats_desc + i * size_desc; > - ret = pread(stats_fd, stats_data, > - pdesc->size * sizeof(*stats_data), > - header.data_offset + size_data); > - TEST_ASSERT(ret == pdesc->size * sizeof(*stats_data), > - "Read data of KVM stats: %s", pdesc->name); > - size_data += pdesc->size * sizeof(*stats_data); > + read_stat_data(stats_fd, &header, pdesc, stats_data, > + ARRAY_SIZE(stats_data)); > } > > - free(stats_data); > free(stats_desc); > free(id); > } > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index e3ae26fbef03..64e2085f1129 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -2593,3 +2593,24 @@ void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > TEST_ASSERT(ret == stats_descs_size(header), > "Read KVM stats descriptors"); > } > + > +int read_stat_data(int stats_fd, struct kvm_stats_header *header, I would like to keep up the practice of adding docstrings to functions in kvm_util. Can you add docstring comments for this function and the other kvm_util functions introduced by this series? > + struct kvm_stats_desc *desc, uint64_t *data, > + ssize_t max_elements) > +{ > + ssize_t ret; > + > + TEST_ASSERT(desc->size <= max_elements, > + "Max data elements should be at least as large as stat data"); What is the reason for this assertion? Callers are required to read all the data elements of a given stat? > + > + ret = pread(stats_fd, data, desc->size * sizeof(*data), > + header->data_offset + desc->offset); > + > + /* ret from pread is in bytes. */ > + ret = ret / sizeof(*data); > + > + TEST_ASSERT(ret == desc->size, > + "Read data of KVM stats: %s", desc->name); Won't this assertion fail when called from kvm_binary_stats_test.c? kvm_binary_stats_test.c looks like it reads all the stat data at once, which means ret will be the total number of stat data points, and desc->size will be the number of stat data points in the first stat. > + > + return ret; > +} > -- > 2.35.1.1178.g4f1659d476-goog >
On Mon, Apr 11, 2022, Ben Gardon wrote: > Move the code to read the binary stats data to the KVM selftests > library. It will be re-used by other tests to check KVM behavior. > > No functional change intended. > > Signed-off-by: Ben Gardon <bgardon@google.com> Reviewed-by: Mingwei Zhang <mizhang@google.com> > --- > .../selftests/kvm/include/kvm_util_base.h | 3 +++ > .../selftests/kvm/kvm_binary_stats_test.c | 20 +++++------------- > tools/testing/selftests/kvm/lib/kvm_util.c | 21 +++++++++++++++++++ > 3 files changed, 29 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index c5f34551ff76..b2684cfc2cb1 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -405,6 +405,9 @@ struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > struct kvm_stats_header *header); > void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > struct kvm_stats_desc *stats_desc); > +int read_stat_data(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *desc, uint64_t *data, > + ssize_t max_elements); > > uint32_t guest_get_vcpuid(void); > > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > index e4795bad7db6..97b180249ba0 100644 > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > @@ -20,6 +20,8 @@ > #include "asm/kvm.h" > #include "linux/kvm.h" > > +#define STAT_MAX_ELEMENTS 1000 > + > static void stats_test(int stats_fd) > { > ssize_t ret; > @@ -29,7 +31,7 @@ static void stats_test(int stats_fd) > struct kvm_stats_header header; > char *id; > struct kvm_stats_desc *stats_desc; > - u64 *stats_data; > + u64 stats_data[STAT_MAX_ELEMENTS]; > struct kvm_stats_desc *pdesc; > > /* Read kvm stats header */ > @@ -130,25 +132,13 @@ static void stats_test(int stats_fd) > pdesc->offset, pdesc->name); > } > > - /* Allocate memory for stats data */ > - stats_data = malloc(size_data); > - TEST_ASSERT(stats_data, "Allocate memory for stats data"); > - /* Read kvm stats data as a bulk */ > - ret = pread(stats_fd, stats_data, size_data, header.data_offset); > - TEST_ASSERT(ret == size_data, "Read KVM stats data"); > /* Read kvm stats data one by one */ > - size_data = 0; > for (i = 0; i < header.num_desc; ++i) { > pdesc = (void *)stats_desc + i * size_desc; > - ret = pread(stats_fd, stats_data, > - pdesc->size * sizeof(*stats_data), > - header.data_offset + size_data); > - TEST_ASSERT(ret == pdesc->size * sizeof(*stats_data), > - "Read data of KVM stats: %s", pdesc->name); > - size_data += pdesc->size * sizeof(*stats_data); > + read_stat_data(stats_fd, &header, pdesc, stats_data, > + ARRAY_SIZE(stats_data)); > } > > - free(stats_data); > free(stats_desc); > free(id); > } > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index e3ae26fbef03..64e2085f1129 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -2593,3 +2593,24 @@ void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > TEST_ASSERT(ret == stats_descs_size(header), > "Read KVM stats descriptors"); > } > + > +int read_stat_data(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *desc, uint64_t *data, > + ssize_t max_elements) > +{ > + ssize_t ret; > + > + TEST_ASSERT(desc->size <= max_elements, > + "Max data elements should be at least as large as stat data"); > + > + ret = pread(stats_fd, data, desc->size * sizeof(*data), > + header->data_offset + desc->offset); hmm, by checking the comments in kvm.h, I cannot infer desc->offset should be added with header->data_offset. So, could you add the following commit into your series. I feel that we need to improve readability a little bit. Existing comments for kvm_stats_desc is not quite helpful for users. From: Mingwei Zhang <mizhang@google.com> Date: Tue, 12 Apr 2022 01:09:10 +0000 Subject: [PATCH] KVM: stats: improve readability of kvm_stats_header and kvm_stats_desc Some comments on these data structures are not quite helpful from user perspective. For instance, kvm_stats_header->name_size was shared across all stats. This might be a little bit counter-intuitive, since each stat should have a different name length. So it has to be the maximum length of the stats string. We cannot infer that by just reading the comments without going through the implementation. In addition, kvm_stat_desc->offset is a relative offset base on kvm_stats_header->data_offset. It is better to call it out clearly in comments. Update the comments of the fields in these two data structures. Cc: Jing Zhang <zhangjingos@google.com> Signed-off-by: Mingwei Zhang <mizhang@google.com> --- include/uapi/linux/kvm.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 5c0d7c77e9bd..986728b5b3fb 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1987,7 +1987,7 @@ struct kvm_dirty_gfn { /** * struct kvm_stats_header - Header of per vm/vcpu binary statistics data. * @flags: Some extra information for header, always 0 for now. - * @name_size: The size in bytes of the memory which contains statistics + * @name_size: The maximum size in bytes of the memory which contains statistics * name string including trailing '\0'. The memory is allocated * at the send of statistics descriptor. * @num_desc: The number of statistics the vm or vcpu has. @@ -2042,7 +2042,8 @@ struct kvm_stats_header { * @size: The number of data items for this stats. * Every data item is of type __u64. * @offset: The offset of the stats to the start of stat structure in - * structure kvm or kvm_vcpu. + * structure kvm or kvm_vcpu. When using it, add its value with + * kvm_stats_header->data_offset. * @bucket_size: A parameter value used for histogram stats. It is only used * for linear histogram stats, specifying the size of the bucket; * @name: The name string for the stats. Its size is indicated by the base-commit: ce522ba9ef7e2d9fb22a39eb3371c0c64e2a433e -- 2.35.1.1178.g4f1659d476-goog > + > + /* ret from pread is in bytes. */ > + ret = ret / sizeof(*data); > + > + TEST_ASSERT(ret == desc->size, > + "Read data of KVM stats: %s", desc->name); > + > + return ret; > +} > -- > 2.35.1.1178.g4f1659d476-goog >
On Mon, Apr 11, 2022 at 3:15 PM David Matlack <dmatlack@google.com> wrote: > > On Mon, Apr 11, 2022 at 2:10 PM Ben Gardon <bgardon@google.com> wrote: > > > > Move the code to read the binary stats data to the KVM selftests > > library. It will be re-used by other tests to check KVM behavior. > > > > No functional change intended. > > > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > .../selftests/kvm/include/kvm_util_base.h | 3 +++ > > .../selftests/kvm/kvm_binary_stats_test.c | 20 +++++------------- > > tools/testing/selftests/kvm/lib/kvm_util.c | 21 +++++++++++++++++++ > > 3 files changed, 29 insertions(+), 15 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > index c5f34551ff76..b2684cfc2cb1 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > @@ -405,6 +405,9 @@ struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > > struct kvm_stats_header *header); > > void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > > struct kvm_stats_desc *stats_desc); > > +int read_stat_data(int stats_fd, struct kvm_stats_header *header, > > + struct kvm_stats_desc *desc, uint64_t *data, > > + ssize_t max_elements); > > > > uint32_t guest_get_vcpuid(void); > > > > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > index e4795bad7db6..97b180249ba0 100644 > > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > @@ -20,6 +20,8 @@ > > #include "asm/kvm.h" > > #include "linux/kvm.h" > > > > +#define STAT_MAX_ELEMENTS 1000 > > + > > static void stats_test(int stats_fd) > > { > > ssize_t ret; > > @@ -29,7 +31,7 @@ static void stats_test(int stats_fd) > > struct kvm_stats_header header; > > char *id; > > struct kvm_stats_desc *stats_desc; > > - u64 *stats_data; > > + u64 stats_data[STAT_MAX_ELEMENTS]; > > What is the benefit of changing stats_data to a stack allocation with > a fixed limit? There isn't really a benefit. Will remove. > > > struct kvm_stats_desc *pdesc; > > > > /* Read kvm stats header */ > > @@ -130,25 +132,13 @@ static void stats_test(int stats_fd) > > pdesc->offset, pdesc->name); > > } > > > > - /* Allocate memory for stats data */ > > - stats_data = malloc(size_data); > > - TEST_ASSERT(stats_data, "Allocate memory for stats data"); > > - /* Read kvm stats data as a bulk */ > > - ret = pread(stats_fd, stats_data, size_data, header.data_offset); > > - TEST_ASSERT(ret == size_data, "Read KVM stats data"); > > /* Read kvm stats data one by one */ > > - size_data = 0; > > for (i = 0; i < header.num_desc; ++i) { > > pdesc = (void *)stats_desc + i * size_desc; > > - ret = pread(stats_fd, stats_data, > > - pdesc->size * sizeof(*stats_data), > > - header.data_offset + size_data); > > - TEST_ASSERT(ret == pdesc->size * sizeof(*stats_data), > > - "Read data of KVM stats: %s", pdesc->name); > > - size_data += pdesc->size * sizeof(*stats_data); > > + read_stat_data(stats_fd, &header, pdesc, stats_data, > > + ARRAY_SIZE(stats_data)); > > } > > > > - free(stats_data); > > free(stats_desc); > > free(id); > > } > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > index e3ae26fbef03..64e2085f1129 100644 > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > @@ -2593,3 +2593,24 @@ void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > > TEST_ASSERT(ret == stats_descs_size(header), > > "Read KVM stats descriptors"); > > } > > + > > +int read_stat_data(int stats_fd, struct kvm_stats_header *header, > > I would like to keep up the practice of adding docstrings to functions > in kvm_util. Can you add docstring comments for this function and the > other kvm_util functions introduced by this series? Will do. > > > + struct kvm_stats_desc *desc, uint64_t *data, > > + ssize_t max_elements) > > +{ > > + ssize_t ret; > > + > > + TEST_ASSERT(desc->size <= max_elements, > > + "Max data elements should be at least as large as stat data"); > > What is the reason for this assertion? Callers are required to read > all the data elements of a given stat? Yeah, that was the idea, but it doesn't seem very useful. I'll remove it. > > > + > > + ret = pread(stats_fd, data, desc->size * sizeof(*data), > > + header->data_offset + desc->offset); > > + > > + /* ret from pread is in bytes. */ > > + ret = ret / sizeof(*data); > > + > > + TEST_ASSERT(ret == desc->size, > > + "Read data of KVM stats: %s", desc->name); > > Won't this assertion fail when called from kvm_binary_stats_test.c? > kvm_binary_stats_test.c looks like it reads all the stat data at once, > which means ret will be the total number of stat data points, and > desc->size will be the number of stat data points in the first stat. Hmmm it shouldn't. I think we're just reading one stat at at time. > > > + > > + return ret; > > +} > > -- > > 2.35.1.1178.g4f1659d476-goog > >
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index c5f34551ff76..b2684cfc2cb1 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -405,6 +405,9 @@ struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, struct kvm_stats_header *header); void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, struct kvm_stats_desc *stats_desc); +int read_stat_data(int stats_fd, struct kvm_stats_header *header, + struct kvm_stats_desc *desc, uint64_t *data, + ssize_t max_elements); uint32_t guest_get_vcpuid(void); diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index e4795bad7db6..97b180249ba0 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -20,6 +20,8 @@ #include "asm/kvm.h" #include "linux/kvm.h" +#define STAT_MAX_ELEMENTS 1000 + static void stats_test(int stats_fd) { ssize_t ret; @@ -29,7 +31,7 @@ static void stats_test(int stats_fd) struct kvm_stats_header header; char *id; struct kvm_stats_desc *stats_desc; - u64 *stats_data; + u64 stats_data[STAT_MAX_ELEMENTS]; struct kvm_stats_desc *pdesc; /* Read kvm stats header */ @@ -130,25 +132,13 @@ static void stats_test(int stats_fd) pdesc->offset, pdesc->name); } - /* Allocate memory for stats data */ - stats_data = malloc(size_data); - TEST_ASSERT(stats_data, "Allocate memory for stats data"); - /* Read kvm stats data as a bulk */ - ret = pread(stats_fd, stats_data, size_data, header.data_offset); - TEST_ASSERT(ret == size_data, "Read KVM stats data"); /* Read kvm stats data one by one */ - size_data = 0; for (i = 0; i < header.num_desc; ++i) { pdesc = (void *)stats_desc + i * size_desc; - ret = pread(stats_fd, stats_data, - pdesc->size * sizeof(*stats_data), - header.data_offset + size_data); - TEST_ASSERT(ret == pdesc->size * sizeof(*stats_data), - "Read data of KVM stats: %s", pdesc->name); - size_data += pdesc->size * sizeof(*stats_data); + read_stat_data(stats_fd, &header, pdesc, stats_data, + ARRAY_SIZE(stats_data)); } - free(stats_data); free(stats_desc); free(id); } diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index e3ae26fbef03..64e2085f1129 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -2593,3 +2593,24 @@ void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, TEST_ASSERT(ret == stats_descs_size(header), "Read KVM stats descriptors"); } + +int read_stat_data(int stats_fd, struct kvm_stats_header *header, + struct kvm_stats_desc *desc, uint64_t *data, + ssize_t max_elements) +{ + ssize_t ret; + + TEST_ASSERT(desc->size <= max_elements, + "Max data elements should be at least as large as stat data"); + + ret = pread(stats_fd, data, desc->size * sizeof(*data), + header->data_offset + desc->offset); + + /* ret from pread is in bytes. */ + ret = ret / sizeof(*data); + + TEST_ASSERT(ret == desc->size, + "Read data of KVM stats: %s", desc->name); + + return ret; +}
Move the code to read the binary stats data to the KVM selftests library. It will be re-used by other tests to check KVM behavior. No functional change intended. Signed-off-by: Ben Gardon <bgardon@google.com> --- .../selftests/kvm/include/kvm_util_base.h | 3 +++ .../selftests/kvm/kvm_binary_stats_test.c | 20 +++++------------- tools/testing/selftests/kvm/lib/kvm_util.c | 21 +++++++++++++++++++ 3 files changed, 29 insertions(+), 15 deletions(-)