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
> > >
