On Tue, May 03, 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.
>
> Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>
> Reviewed-by: Peter Xu <peterx@xxxxxxxxxx>
> Signed-off-by: Ben Gardon <bgardon@xxxxxxxxxx>
> ---
> .../selftests/kvm/include/kvm_util_base.h | 3 ++
> .../selftests/kvm/kvm_binary_stats_test.c | 7 ++--
> tools/testing/selftests/kvm/lib/kvm_util.c | 36 +++++++++++++++++++
> 3 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index fabe46ddc1b2..2a3a4d9ed8e3 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -403,6 +403,9 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
> void read_stats_header(int stats_fd, struct kvm_stats_header *header);
> struct kvm_stats_desc *read_stats_desc(int stats_fd,
> struct kvm_stats_header *header);
> +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 8b31f8fc7e08..59677fae26e5 100644
> --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> @@ -160,11 +160,8 @@ static void stats_test(int stats_fd)
> 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);
> + read_stat_data(stats_fd, &header, pdesc, stats_data,
> + pdesc->size);
> size_data += pdesc->size * sizeof(*stats_data);
Not your code, but updating size_data is pointless and confusing. It's especially
confusing as of this patch because it ignores the return read_stat_data(). I vote
to opportunistically delete this code.
> }
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 12fa8cc88043..ea4ab64e5997 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2615,3 +2615,39 @@ struct kvm_stats_desc *read_stats_desc(int stats_fd,
>
> return stats_desc;
> }
> +
> +/*
> + * Read stat data for a particular stat
> + *
> + * Input Args:
> + * stats_fd - the file descriptor for the binary stats file from which to read
> + * header - the binary stats metadata header corresponding to the given FD
> + * desc - the binary stat metadata for the particular stat to be read
> + * max_elements - the maximum number of 8-byte values to read into data
> + *
> + * Output Args:
> + * data - the buffer into which stat data should be read
> + *
> + * Return:
> + * The number of data elements read into data or -ERRNO on error.
This is a lie, it can never return -ERRNO. Well, unless the caller is mean and
passes in -EINVAL for max_elements I guess.
> + *
> + * Read the data values of a specified stat from the binary stats interface.
> + */
> +int read_stat_data(int stats_fd, struct kvm_stats_header *header,
> + struct kvm_stats_desc *desc, uint64_t *data,
> + ssize_t max_elements)
Uber nit, @max_elements should be unsigned size_t, only the return from pread()
is signed, the input is unsigned.
> +{
> + ssize_t size = min_t(ssize_t, desc->size, max_elements);
Not your fault (I blame struct kvm_stats_desc), but "nr_elements" would be far
more appropriate than "size". And that frees up "size" to be the actual size,
which eliminates the division.
> + ssize_t ret;
> +
> + ret = pread(stats_fd, data, size * sizeof(*data),
> + header->data_offset + desc->offset);
> +
> + /* ret from pread is in bytes. */
> + ret = ret / sizeof(*data);
> +
> + TEST_ASSERT(ret == size,
> + "Read data of KVM stats: %s", desc->name);
It'd be very helpful to print the expected vs. actual.
> +
> + return ret;
Eww. I really, really hate code that asserts on a value and then returns that
same value. E.g. looking at just the declaration of read_stat_data() and the
change in stats_test(), I genuinely thought this patch dropped the assert. The
assert in vm_get_stat() also added to the confusion (I was reviewing that patch,
not this one).
Rather than return the number of entries read, just assert that the number of
elements to be read is non-zero, then vm_get_stat() doesn't need to assert because
it'll be impossible to read anything but one entry without asserting.
void read_stat_data(int stats_fd, struct kvm_stats_header *header,
struct kvm_stats_desc *desc, uint64_t *data,
size_t max_elements)
{
size_t nr_elements = min_t(size_t, desc->size, max_elements);
size_t size = nr_elements * sizeof(*data);
ssize_t ret;
TEST_ASSERT(size, "No elements in stat '%s'", desc->name);
ret = pread(stats_fd, data, size, header->data_offset + desc->offset);
TEST_ASSERT(ret == size,
"pread() failed on stat '%s', wanted %lu bytes, got %ld",
desc->name, size, ret);
}
