From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6BEF0C433EF for ; Thu, 5 May 2022 18:06:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1382987AbiEESKA (ORCPT ); Thu, 5 May 2022 14:10:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56772 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1347541AbiEESJ6 (ORCPT ); Thu, 5 May 2022 14:09:58 -0400 Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B3CC03A714 for ; Thu, 5 May 2022 11:06:18 -0700 (PDT) Received: by mail-pj1-x102a.google.com with SMTP id r9so4880576pjo.5 for ; Thu, 05 May 2022 11:06:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=csa5Mj5E+ZKvHVZb+3IE/GuCgzkqVA+V6IRZnVKxYjQ=; b=BDoPQjE5lYzOYbFxxPfMObdUUb96/hnlzMC2QQ3J9l8h+pnj7Akud0FVhawwEah6R2 NtYhsKmxx7l2g8bQrYlZ2iHixqIqRhvXrDqjQAAaDtErX4k1eQ2ccAFSuEa+2jVL7RR8 fDbSJ76yzER0JwcZVw7fzLN5KkV/V3hRa2RFgmuDBXgj9zdge/MdYmLaFFET1+dfTglS I6T8gyYdDI58sHz0V9F6duONXsoeCNJy5+5CYd+vu++4guK+roeUCnxPyTOHRf32oq5w NGT+/AXzUaKPKT5atnwbCpk19p5KUrGrn9vT6xwMtAX7ZJN21ZKSproidUMh93/oQg6o Cxlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=csa5Mj5E+ZKvHVZb+3IE/GuCgzkqVA+V6IRZnVKxYjQ=; b=mtdksHsk21aSHelm0xuM9hgVvqlzKhlaDrUaHl6+LL2AaRFEwt2dhPDbEkvKQxxv5V 0HO0DySO/HMe/10Cs0Dv4cLeWGbGiU56tg3Q3Z3wMUQJFLr44MQKTpui3qLfx16+NveD MMwAuaL4gOB7RhY6gU2Y+A7qMmh9G905eH2Jm3KCAyNzotzhJEj4qpEUJaScOVSrgrxw Nr3naISi6i+2TbJ9taQuLZT4YRJHegCfNIw6ObBFPx/bgTrI+0XNE+8FmDBgeHsPIdEZ mVPYQTlj2TdhJEBt6/qonHxdm+QWDkj+eGRA32J0VGrEmhCZTY1Hremo/uJvokILU33F +TFw== X-Gm-Message-State: AOAM5319clPs3P5VhiD8eJjm6wQG6ytQnM5G+WRqU4yuLxjZTtN2bvcu XvkGZkdi3oRVTQ8aUhIXNgNLGw== X-Google-Smtp-Source: ABdhPJwNktaPZjslvuxXyp8B21Tg5wZCqPH78HxSFnAeo0EZwJhjpESx7WUTNVNgOEdA6HqZWY1BZg== X-Received: by 2002:a17:90b:4a4e:b0:1dc:55ca:6f33 with SMTP id lb14-20020a17090b4a4e00b001dc55ca6f33mr7653761pjb.4.1651773978039; Thu, 05 May 2022 11:06:18 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id d133-20020a621d8b000000b0050dc7628196sm1671225pfd.112.2022.05.05.11.06.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 May 2022 11:06:17 -0700 (PDT) Date: Thu, 5 May 2022 18:06:14 +0000 From: Sean Christopherson To: Ben Gardon Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paolo Bonzini , Peter Xu , David Matlack , Jim Mattson , David Dunn , Jing Zhang , Junaid Shahid Subject: Re: [PATCH v7 05/11] KVM: selftests: Read binary stat data in lib Message-ID: References: <20220503183045.978509-1-bgardon@google.com> <20220503183045.978509-6-bgardon@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220503183045.978509-6-bgardon@google.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Reviewed-by: Peter Xu > Signed-off-by: Ben Gardon > --- > .../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); }