Re: [PATCH 4/4 v2] KVM: x86: Add a new VM statistic to show number of VCPUs created in a given VM

[Sean Christopherson <seanjc@google.com>]

On Fri, May 21, 2021, Krish Sadhukhan wrote:
> 
> On 5/20/21 8:04 AM, Sean Christopherson wrote:
> > On Wed, May 19, 2021, Krish Sadhukhan wrote:
> > > 'struct kvm' already has a member for counting the number of VCPUs created
> > > for a given VM. Add this as a new VM statistic to KVM debugfs.
> > Huh!??  Why?  Userspace is the one creating the vCPUs, it darn well should know
> > how many it's created.
> 
> If I am providing a host for users to create VMs, how do I know who creates
> how many VCPUs ? This statistic is intended show usage of VCPU resources on
> a host used by customers.

How are reviewers supposed to know that that's the use case?  Use the changelog
to state _why_ a patch is needed/justified.

> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index cbca3609a152..a9d27ce4cc93 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -258,6 +258,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
> > >   	VM_STAT("nx_largepages_splitted", nx_lpage_splits, .mode = 0444),
> > >   	VM_STAT("max_mmu_page_hash_collisions", max_mmu_page_hash_collisions),
> > >   	VM_STAT("vcpus_ran_nested", vcpus_ran_nested),
> > > +	VM_STAT("created_vcpus", created_vcpus),

IMO, the "created" part is unnecessary for the stats, i.e. just call it "vcpus",
or maybe "nr_vcpus".

> > >   	{ NULL }
> > >   };
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 6b4feb92dc79..ac8f02d8a051 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -3318,6 +3318,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> > >   	}
> > >   	kvm->created_vcpus++;
> > > +	kvm->stat.created_vcpus++;
> > >   	mutex_unlock(&kvm->lock);
> > >   	r = kvm_arch_vcpu_precreate(kvm, id);
> > > @@ -3394,6 +3395,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> > >   vcpu_decrement:
> > >   	mutex_lock(&kvm->lock);
> > >   	kvm->created_vcpus--;
> > > +	kvm->stat.created_vcpus--;
> > >   	mutex_unlock(&kvm->lock);
> > >   	return r;
> > >   }
> > > -- 
> > > 2.27.0
> > >