Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 2 Aug 2021 23:02:05 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Mingwei Zhang <mizhang@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Ben Gardon <bgardon@google.com>,
        Jing Zhang <jingzhangos@google.com>,
        David Matlack <dmatlack@google.com>
Subject: Re: [PATCH v3 3/3] KVM: x86/mmu: Add detailed page size stats
Message-ID: <YQh5baC5cYAuj6y0@google.com>
References: <20210730225939.3852712-1-mizhang@google.com>
 <20210730225939.3852712-4-mizhang@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210730225939.3852712-4-mizhang@google.com>
Precedence: bulk

On Fri, Jul 30, 2021, Mingwei Zhang wrote:
> Existing KVM code tracks the number of large pages regardless of their
> sizes. Therefore, when large page of 1GB (or larger) is adopted, the
> information becomes less useful because lpages counts a mix of 1G and 2M
> pages.
> 
> So remove the lpages since it is easy for user space to aggregate the info.
> Instead, provide a comprehensive page stats of all sizes from 4K to 512G.
> 
> Suggested-by: Ben Gardon <bgardon@google.com>
> Reviewed-by: Ben Gardon <bgardon@google.com>
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> Cc: Jing Zhang <jingzhangos@google.com>
> Cc: David Matlack <dmatlack@google.com>
> Cc: Sean Christopherson <seanjc@google.com>

Looks good, but you'll probably need/want to rebase to kvm/queue once that settles
down (I suspect a forced push is coming this week).  This has quite a few conflicts
with other stuff sitting in kvm/queue.

> ---
>  arch/x86/include/asm/kvm_host.h | 10 +++++++++-
>  arch/x86/kvm/mmu.h              |  4 ++++
>  arch/x86/kvm/mmu/mmu.c          | 26 +++++++++++++-------------
>  arch/x86/kvm/mmu/tdp_mmu.c      | 15 ++-------------
>  arch/x86/kvm/x86.c              |  7 +++++--
>  5 files changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 974cbfb1eefe..eb6edc36b3ed 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1206,9 +1206,17 @@ struct kvm_vm_stat {
>  	u64 mmu_recycled;
>  	u64 mmu_cache_miss;
>  	u64 mmu_unsync;
> -	u64 lpages;
>  	u64 nx_lpage_splits;
>  	u64 max_mmu_page_hash_collisions;
> +	union {
> +		struct {
> +			atomic64_t pages_4k;
> +			atomic64_t pages_2m;
> +			atomic64_t pages_1g;
> +			atomic64_t pages_512g;
> +		};
> +		atomic64_t pages[4];
> +	};

...

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8166ad113fb2..e4dfcd5d83ad 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -235,9 +235,12 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
>  	STATS_DESC_COUNTER(VM, mmu_recycled),
>  	STATS_DESC_COUNTER(VM, mmu_cache_miss),
>  	STATS_DESC_ICOUNTER(VM, mmu_unsync),
> -	STATS_DESC_ICOUNTER(VM, lpages),
>  	STATS_DESC_ICOUNTER(VM, nx_lpage_splits),
> -	STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions)
> +	STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions),
> +	STATS_DESC_ICOUNTER(VM, pages_4k),
> +	STATS_DESC_ICOUNTER(VM, pages_2m),
> +	STATS_DESC_ICOUNTER(VM, pages_1g),
> +	STATS_DESC_ICOUNTER(VM, pages_512g)

Uber nit that I wouldn't even have noticed if this didn't conflict, but there's
no need to land the union and the stats definitions at the end of the structs,
i.e. the new fields can directly replace lpages.  I don't think it will actually
avoid a conflict, but it would avoid modifying the max_mmu_page_hash_collisions
line.

>  };
>  static_assert(ARRAY_SIZE(kvm_vm_stats_desc) ==
>  		sizeof(struct kvm_vm_stat) / sizeof(u64));
> -- 
> 2.32.0.554.ge1b32706d8-goog
>