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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 33FB6C433E6 for ; Wed, 10 Mar 2021 14:19:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 054D164FFA for ; Wed, 10 Mar 2021 14:19:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232747AbhCJOTR (ORCPT ); Wed, 10 Mar 2021 09:19:17 -0500 Received: from mail.kernel.org ([198.145.29.99]:36280 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230373AbhCJOTK (ORCPT ); Wed, 10 Mar 2021 09:19:10 -0500 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id EF53364FEF; Wed, 10 Mar 2021 14:19:09 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1lJzg3-000mQq-PG; Wed, 10 Mar 2021 14:19:07 +0000 Date: Wed, 10 Mar 2021 14:19:06 +0000 Message-ID: <878s6vxfad.wl-maz@kernel.org> From: Marc Zyngier To: Jing Zhang Cc: KVM , KVM ARM , Linux MIPS , KVM PPC , Linux S390 , Linux kselftest , Paolo Bonzini , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Christian Borntraeger , Janosch Frank , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes , Emanuele Giuseppe Esposito Subject: Re: [RFC PATCH 1/4] KVM: stats: Separate statistics name strings from debugfs code In-Reply-To: <20210310003024.2026253-2-jingzhangos@google.com> References: <20210310003024.2026253-1-jingzhangos@google.com> <20210310003024.2026253-2-jingzhangos@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: jingzhangos@google.com, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm-ppc@vger.kernel.org, linux-s390@vger.kernel.org, linux-kselftest@vger.kernel.org, pbonzini@redhat.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, will@kernel.org, chenhuacai@kernel.org, aleksandar.qemu.devel@gmail.com, tsbogend@alpha.franken.de, paulus@ozlabs.org, borntraeger@de.ibm.com, frankja@linux.ibm.com, david@redhat.com, cohuck@redhat.com, imbrenda@linux.ibm.com, seanjc@google.com, vkuznets@redhat.com, jmattson@google.com, pshier@google.com, oupton@google.com, rientjes@google.com, eesposit@redhat.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-s390@vger.kernel.org Hi Jing, On Wed, 10 Mar 2021 00:30:21 +0000, Jing Zhang wrote: > > Prepare the statistics name strings for supporting binary format > aggregated statistics data retrieval. > > No functional change intended. > > Signed-off-by: Jing Zhang > --- > arch/arm64/kvm/guest.c | 47 ++++-- > arch/mips/kvm/mips.c | 114 ++++++++++---- > arch/powerpc/kvm/book3s.c | 107 +++++++++---- > arch/powerpc/kvm/booke.c | 84 +++++++--- > arch/s390/kvm/kvm-s390.c | 320 ++++++++++++++++++++++++++------------ > arch/x86/kvm/x86.c | 127 ++++++++++----- > include/linux/kvm_host.h | 31 +++- > 7 files changed, 589 insertions(+), 241 deletions(-) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 9bbd30e62799..fb3aafe76b52 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -28,19 +28,42 @@ > > #include "trace.h" > > +const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN] = { > + "remote_tlb_flush", > +}; > +static_assert(sizeof(kvm_vm_stat_strings) == > + VM_STAT_COUNT * KVM_STATS_NAME_LEN); > + > +const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN] = { > + "halt_successful_poll", > + "halt_attempted_poll", > + "halt_poll_success_ns", > + "halt_poll_fail_ns", > + "halt_poll_invalid", > + "halt_wakeup", > + "hvc_exit_stat", > + "wfe_exit_stat", > + "wfi_exit_stat", > + "mmio_exit_user", > + "mmio_exit_kernel", > + "exits", > +}; > +static_assert(sizeof(kvm_vcpu_stat_strings) == > + VCPU_STAT_COUNT * KVM_STATS_NAME_LEN); > + > struct kvm_stats_debugfs_item debugfs_entries[] = { > - VCPU_STAT("halt_successful_poll", halt_successful_poll), > - VCPU_STAT("halt_attempted_poll", halt_attempted_poll), > - VCPU_STAT("halt_poll_invalid", halt_poll_invalid), > - VCPU_STAT("halt_wakeup", halt_wakeup), > - VCPU_STAT("hvc_exit_stat", hvc_exit_stat), > - VCPU_STAT("wfe_exit_stat", wfe_exit_stat), > - VCPU_STAT("wfi_exit_stat", wfi_exit_stat), > - VCPU_STAT("mmio_exit_user", mmio_exit_user), > - VCPU_STAT("mmio_exit_kernel", mmio_exit_kernel), > - VCPU_STAT("exits", exits), > - VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns), > - VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns), > + VCPU_STAT(halt_successful_poll), > + VCPU_STAT(halt_attempted_poll), > + VCPU_STAT(halt_poll_invalid), > + VCPU_STAT(halt_wakeup), > + VCPU_STAT(hvc_exit_stat), > + VCPU_STAT(wfe_exit_stat), > + VCPU_STAT(wfi_exit_stat), > + VCPU_STAT(mmio_exit_user), > + VCPU_STAT(mmio_exit_kernel), > + VCPU_STAT(exits), > + VCPU_STAT(halt_poll_success_ns), > + VCPU_STAT(halt_poll_fail_ns), So we now have two arrays that can easily deviate in their order, whilst we didn't have that risk before. What is the advantage of doing this? The commit message doesn't really say... [...] > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 1b65e7204344..1ea297458306 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1162,6 +1162,18 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa) > return kvm_is_error_hva(hva); > } > > +#define VM_STAT_COUNT (sizeof(struct kvm_vm_stat)/sizeof(ulong)) > +#define VCPU_STAT_COUNT (sizeof(struct kvm_vcpu_stat)/sizeof(u64)) > +#define KVM_STATS_NAME_LEN 32 > + > +/* Make sure it is synced with fields in struct kvm_vm_stat. */ > +extern const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN]; > +/* Make sure it is synced with fields in struct kvm_vcpu_stat. */ > +extern const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN]; > + > +#define VM_STAT_NAME(id) (kvm_vm_stat_strings[id]) > +#define VCPU_STAT_NAME(id) (kvm_vcpu_stat_strings[id]) > + > enum kvm_stat_kind { > KVM_STAT_VM, > KVM_STAT_VCPU, > @@ -1182,10 +1194,21 @@ struct kvm_stats_debugfs_item { > #define KVM_DBGFS_GET_MODE(dbgfs_item) \ > ((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644) > > -#define VM_STAT(n, x, ...) \ > - { n, offsetof(struct kvm, stat.x), KVM_STAT_VM, ## __VA_ARGS__ } > -#define VCPU_STAT(n, x, ...) \ > - { n, offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU, ## __VA_ARGS__ } > +#define VM_STAT(x, ...) \ > + { \ > + VM_STAT_NAME(offsetof(struct kvm_vm_stat, x)/sizeof(ulong)), \ > + offsetof(struct kvm, stat.x), \ > + KVM_STAT_VM, \ > + ## __VA_ARGS__ \ > + } > + > +#define VCPU_STAT(x, ...) \ > + { \ > + VCPU_STAT_NAME(offsetof(struct kvm_vcpu_stat, x)/sizeof(u64)), \ > + offsetof(struct kvm_vcpu, stat.x), \ > + KVM_STAT_VCPU, \ > + ## __VA_ARGS__ \ > + } Is there any reason why we want to keep kvm_vm_stat populated with ulong, while kvm_vcpu_stat is populated with u64? I have the feeling that this is a fairly pointless difference, and that some of the macros could be unified. Also, using names initialisers would help with the readability of the macros. Thanks, M. -- Without deviation from the norm, progress is not possible. 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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A3A17C433DB for ; Wed, 10 Mar 2021 14:19:18 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 1807B64FF4 for ; Wed, 10 Mar 2021 14:19:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1807B64FF4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 9397A4B35D; Wed, 10 Mar 2021 09:19:17 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qKRG0arxsatB; Wed, 10 Mar 2021 09:19:15 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 56CBE4B372; Wed, 10 Mar 2021 09:19:15 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E97494B367 for ; Wed, 10 Mar 2021 09:19:13 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id HbJIscFOSRiJ for ; Wed, 10 Mar 2021 09:19:12 -0500 (EST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id ABCF64B35A for ; Wed, 10 Mar 2021 09:19:12 -0500 (EST) Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id EF53364FEF; Wed, 10 Mar 2021 14:19:09 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1lJzg3-000mQq-PG; Wed, 10 Mar 2021 14:19:07 +0000 Date: Wed, 10 Mar 2021 14:19:06 +0000 Message-ID: <878s6vxfad.wl-maz@kernel.org> From: Marc Zyngier To: Jing Zhang Subject: Re: [RFC PATCH 1/4] KVM: stats: Separate statistics name strings from debugfs code In-Reply-To: <20210310003024.2026253-2-jingzhangos@google.com> References: <20210310003024.2026253-1-jingzhangos@google.com> <20210310003024.2026253-2-jingzhangos@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: jingzhangos@google.com, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm-ppc@vger.kernel.org, linux-s390@vger.kernel.org, linux-kselftest@vger.kernel.org, pbonzini@redhat.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, will@kernel.org, chenhuacai@kernel.org, aleksandar.qemu.devel@gmail.com, tsbogend@alpha.franken.de, paulus@ozlabs.org, borntraeger@de.ibm.com, frankja@linux.ibm.com, david@redhat.com, cohuck@redhat.com, imbrenda@linux.ibm.com, seanjc@google.com, vkuznets@redhat.com, jmattson@google.com, pshier@google.com, oupton@google.com, rientjes@google.com, eesposit@redhat.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Cc: KVM , David Hildenbrand , Paul Mackerras , Linux kselftest , Claudio Imbrenda , Will Deacon , KVM ARM , Emanuele Giuseppe Esposito , Linux S390 , Janosch Frank , Oliver Upton , Huacai Chen , Christian Borntraeger , Aleksandar Markovic , David Rientjes , KVM PPC , Jim Mattson , Thomas Bogendoerfer , Sean Christopherson , Cornelia Huck , Peter Shier , Linux MIPS , Paolo Bonzini , Vitaly Kuznetsov X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi Jing, On Wed, 10 Mar 2021 00:30:21 +0000, Jing Zhang wrote: > > Prepare the statistics name strings for supporting binary format > aggregated statistics data retrieval. > > No functional change intended. > > Signed-off-by: Jing Zhang > --- > arch/arm64/kvm/guest.c | 47 ++++-- > arch/mips/kvm/mips.c | 114 ++++++++++---- > arch/powerpc/kvm/book3s.c | 107 +++++++++---- > arch/powerpc/kvm/booke.c | 84 +++++++--- > arch/s390/kvm/kvm-s390.c | 320 ++++++++++++++++++++++++++------------ > arch/x86/kvm/x86.c | 127 ++++++++++----- > include/linux/kvm_host.h | 31 +++- > 7 files changed, 589 insertions(+), 241 deletions(-) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 9bbd30e62799..fb3aafe76b52 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -28,19 +28,42 @@ > > #include "trace.h" > > +const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN] = { > + "remote_tlb_flush", > +}; > +static_assert(sizeof(kvm_vm_stat_strings) == > + VM_STAT_COUNT * KVM_STATS_NAME_LEN); > + > +const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN] = { > + "halt_successful_poll", > + "halt_attempted_poll", > + "halt_poll_success_ns", > + "halt_poll_fail_ns", > + "halt_poll_invalid", > + "halt_wakeup", > + "hvc_exit_stat", > + "wfe_exit_stat", > + "wfi_exit_stat", > + "mmio_exit_user", > + "mmio_exit_kernel", > + "exits", > +}; > +static_assert(sizeof(kvm_vcpu_stat_strings) == > + VCPU_STAT_COUNT * KVM_STATS_NAME_LEN); > + > struct kvm_stats_debugfs_item debugfs_entries[] = { > - VCPU_STAT("halt_successful_poll", halt_successful_poll), > - VCPU_STAT("halt_attempted_poll", halt_attempted_poll), > - VCPU_STAT("halt_poll_invalid", halt_poll_invalid), > - VCPU_STAT("halt_wakeup", halt_wakeup), > - VCPU_STAT("hvc_exit_stat", hvc_exit_stat), > - VCPU_STAT("wfe_exit_stat", wfe_exit_stat), > - VCPU_STAT("wfi_exit_stat", wfi_exit_stat), > - VCPU_STAT("mmio_exit_user", mmio_exit_user), > - VCPU_STAT("mmio_exit_kernel", mmio_exit_kernel), > - VCPU_STAT("exits", exits), > - VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns), > - VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns), > + VCPU_STAT(halt_successful_poll), > + VCPU_STAT(halt_attempted_poll), > + VCPU_STAT(halt_poll_invalid), > + VCPU_STAT(halt_wakeup), > + VCPU_STAT(hvc_exit_stat), > + VCPU_STAT(wfe_exit_stat), > + VCPU_STAT(wfi_exit_stat), > + VCPU_STAT(mmio_exit_user), > + VCPU_STAT(mmio_exit_kernel), > + VCPU_STAT(exits), > + VCPU_STAT(halt_poll_success_ns), > + VCPU_STAT(halt_poll_fail_ns), So we now have two arrays that can easily deviate in their order, whilst we didn't have that risk before. What is the advantage of doing this? The commit message doesn't really say... [...] > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 1b65e7204344..1ea297458306 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1162,6 +1162,18 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa) > return kvm_is_error_hva(hva); > } > > +#define VM_STAT_COUNT (sizeof(struct kvm_vm_stat)/sizeof(ulong)) > +#define VCPU_STAT_COUNT (sizeof(struct kvm_vcpu_stat)/sizeof(u64)) > +#define KVM_STATS_NAME_LEN 32 > + > +/* Make sure it is synced with fields in struct kvm_vm_stat. */ > +extern const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN]; > +/* Make sure it is synced with fields in struct kvm_vcpu_stat. */ > +extern const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN]; > + > +#define VM_STAT_NAME(id) (kvm_vm_stat_strings[id]) > +#define VCPU_STAT_NAME(id) (kvm_vcpu_stat_strings[id]) > + > enum kvm_stat_kind { > KVM_STAT_VM, > KVM_STAT_VCPU, > @@ -1182,10 +1194,21 @@ struct kvm_stats_debugfs_item { > #define KVM_DBGFS_GET_MODE(dbgfs_item) \ > ((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644) > > -#define VM_STAT(n, x, ...) \ > - { n, offsetof(struct kvm, stat.x), KVM_STAT_VM, ## __VA_ARGS__ } > -#define VCPU_STAT(n, x, ...) \ > - { n, offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU, ## __VA_ARGS__ } > +#define VM_STAT(x, ...) \ > + { \ > + VM_STAT_NAME(offsetof(struct kvm_vm_stat, x)/sizeof(ulong)), \ > + offsetof(struct kvm, stat.x), \ > + KVM_STAT_VM, \ > + ## __VA_ARGS__ \ > + } > + > +#define VCPU_STAT(x, ...) \ > + { \ > + VCPU_STAT_NAME(offsetof(struct kvm_vcpu_stat, x)/sizeof(u64)), \ > + offsetof(struct kvm_vcpu, stat.x), \ > + KVM_STAT_VCPU, \ > + ## __VA_ARGS__ \ > + } Is there any reason why we want to keep kvm_vm_stat populated with ulong, while kvm_vcpu_stat is populated with u64? I have the feeling that this is a fairly pointless difference, and that some of the macros could be unified. Also, using names initialisers would help with the readability of the macros. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Date: Wed, 10 Mar 2021 14:19:06 +0000 Subject: Re: [RFC PATCH 1/4] KVM: stats: Separate statistics name strings from debugfs code Message-Id: <878s6vxfad.wl-maz@kernel.org> List-Id: References: <20210310003024.2026253-1-jingzhangos@google.com> <20210310003024.2026253-2-jingzhangos@google.com> In-Reply-To: <20210310003024.2026253-2-jingzhangos@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Jing Zhang Cc: KVM , KVM ARM , Linux MIPS , KVM PPC , Linux S390 , Linux kselftest , Paolo Bonzini , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Christian Borntraeger , Janosch Frank , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes , Emanuele Giuseppe Esposito Hi Jing, On Wed, 10 Mar 2021 00:30:21 +0000, Jing Zhang wrote: > > Prepare the statistics name strings for supporting binary format > aggregated statistics data retrieval. > > No functional change intended. > > Signed-off-by: Jing Zhang > --- > arch/arm64/kvm/guest.c | 47 ++++-- > arch/mips/kvm/mips.c | 114 ++++++++++---- > arch/powerpc/kvm/book3s.c | 107 +++++++++---- > arch/powerpc/kvm/booke.c | 84 +++++++--- > arch/s390/kvm/kvm-s390.c | 320 ++++++++++++++++++++++++++------------ > arch/x86/kvm/x86.c | 127 ++++++++++----- > include/linux/kvm_host.h | 31 +++- > 7 files changed, 589 insertions(+), 241 deletions(-) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 9bbd30e62799..fb3aafe76b52 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -28,19 +28,42 @@ > > #include "trace.h" > > +const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN] = { > + "remote_tlb_flush", > +}; > +static_assert(sizeof(kvm_vm_stat_strings) = > + VM_STAT_COUNT * KVM_STATS_NAME_LEN); > + > +const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN] = { > + "halt_successful_poll", > + "halt_attempted_poll", > + "halt_poll_success_ns", > + "halt_poll_fail_ns", > + "halt_poll_invalid", > + "halt_wakeup", > + "hvc_exit_stat", > + "wfe_exit_stat", > + "wfi_exit_stat", > + "mmio_exit_user", > + "mmio_exit_kernel", > + "exits", > +}; > +static_assert(sizeof(kvm_vcpu_stat_strings) = > + VCPU_STAT_COUNT * KVM_STATS_NAME_LEN); > + > struct kvm_stats_debugfs_item debugfs_entries[] = { > - VCPU_STAT("halt_successful_poll", halt_successful_poll), > - VCPU_STAT("halt_attempted_poll", halt_attempted_poll), > - VCPU_STAT("halt_poll_invalid", halt_poll_invalid), > - VCPU_STAT("halt_wakeup", halt_wakeup), > - VCPU_STAT("hvc_exit_stat", hvc_exit_stat), > - VCPU_STAT("wfe_exit_stat", wfe_exit_stat), > - VCPU_STAT("wfi_exit_stat", wfi_exit_stat), > - VCPU_STAT("mmio_exit_user", mmio_exit_user), > - VCPU_STAT("mmio_exit_kernel", mmio_exit_kernel), > - VCPU_STAT("exits", exits), > - VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns), > - VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns), > + VCPU_STAT(halt_successful_poll), > + VCPU_STAT(halt_attempted_poll), > + VCPU_STAT(halt_poll_invalid), > + VCPU_STAT(halt_wakeup), > + VCPU_STAT(hvc_exit_stat), > + VCPU_STAT(wfe_exit_stat), > + VCPU_STAT(wfi_exit_stat), > + VCPU_STAT(mmio_exit_user), > + VCPU_STAT(mmio_exit_kernel), > + VCPU_STAT(exits), > + VCPU_STAT(halt_poll_success_ns), > + VCPU_STAT(halt_poll_fail_ns), So we now have two arrays that can easily deviate in their order, whilst we didn't have that risk before. What is the advantage of doing this? The commit message doesn't really say... [...] > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 1b65e7204344..1ea297458306 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1162,6 +1162,18 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa) > return kvm_is_error_hva(hva); > } > > +#define VM_STAT_COUNT (sizeof(struct kvm_vm_stat)/sizeof(ulong)) > +#define VCPU_STAT_COUNT (sizeof(struct kvm_vcpu_stat)/sizeof(u64)) > +#define KVM_STATS_NAME_LEN 32 > + > +/* Make sure it is synced with fields in struct kvm_vm_stat. */ > +extern const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN]; > +/* Make sure it is synced with fields in struct kvm_vcpu_stat. */ > +extern const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN]; > + > +#define VM_STAT_NAME(id) (kvm_vm_stat_strings[id]) > +#define VCPU_STAT_NAME(id) (kvm_vcpu_stat_strings[id]) > + > enum kvm_stat_kind { > KVM_STAT_VM, > KVM_STAT_VCPU, > @@ -1182,10 +1194,21 @@ struct kvm_stats_debugfs_item { > #define KVM_DBGFS_GET_MODE(dbgfs_item) \ > ((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644) > > -#define VM_STAT(n, x, ...) \ > - { n, offsetof(struct kvm, stat.x), KVM_STAT_VM, ## __VA_ARGS__ } > -#define VCPU_STAT(n, x, ...) \ > - { n, offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU, ## __VA_ARGS__ } > +#define VM_STAT(x, ...) \ > + { \ > + VM_STAT_NAME(offsetof(struct kvm_vm_stat, x)/sizeof(ulong)), \ > + offsetof(struct kvm, stat.x), \ > + KVM_STAT_VM, \ > + ## __VA_ARGS__ \ > + } > + > +#define VCPU_STAT(x, ...) \ > + { \ > + VCPU_STAT_NAME(offsetof(struct kvm_vcpu_stat, x)/sizeof(u64)), \ > + offsetof(struct kvm_vcpu, stat.x), \ > + KVM_STAT_VCPU, \ > + ## __VA_ARGS__ \ > + } Is there any reason why we want to keep kvm_vm_stat populated with ulong, while kvm_vcpu_stat is populated with u64? I have the feeling that this is a fairly pointless difference, and that some of the macros could be unified. Also, using names initialisers would help with the readability of the macros. Thanks, M. -- Without deviation from the norm, progress is not possible.