Re: [PATCH v4 04/10] KVM: selftests: Read binary stat data in lib
From: Mingwei Zhang
Date: Mon Apr 11 2022 - 21:25:27 EST
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@xxxxxxxxxx>
Reviewed-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
> ---
> .../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@xxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
---
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
>