Re: [PATCH v4 1/4] KVM: Implement dirty quota-based throttling of vcpus

From: Sean Christopherson <seanjc@google.com>
To: Peter Xu <peterx@redhat.com>
Cc: Shivam Kumar <shivam.kumar1@nutanix.com>,
	Marc Zyngier <maz@kernel.org>,
	pbonzini@redhat.com, james.morse@arm.com,
	borntraeger@linux.ibm.com, david@redhat.com, kvm@vger.kernel.org,
	Shaju Abraham <shaju.abraham@nutanix.com>,
	Manish Mishra <manish.mishra@nutanix.com>,
	Anurag Madnawat <anurag.madnawat@nutanix.com>
Subject: Re: [PATCH v4 1/4] KVM: Implement dirty quota-based throttling of vcpus
Date: Fri, 15 Jul 2022 22:38:36 +0000	[thread overview]
Message-ID: <YtHsbBbwE4iAsfG5@google.com> (raw)
In-Reply-To: <YtGjQNyQcN3GiVgS@xz-m1.local>

On Fri, Jul 15, 2022, Peter Xu wrote:
> On Fri, Jul 15, 2022 at 05:07:45PM +0000, Sean Christopherson wrote:
> > On Fri, Jul 15, 2022, Peter Xu wrote:
> > > On Fri, Jul 15, 2022 at 04:23:54PM +0000, Sean Christopherson wrote:
> > > > And the reasoning behind not having kvm_run.dirty_count is that it's fully
> > > > redundant if KVM provides a stat, and IMO such a stat will be quite helpful for
> > > > things beyond dirty quotas, e.g. being able to see which vCPUs are dirtying memory
> > > > from the command line for debug purposes.
> > > 
> > > Not if with overflow in mind?  Note that I totally agree the overflow may
> > > not even happen, but I think it makes sense to consider as a complete
> > > design of ceiling-based approach.  Think the Millennium bug, we never know
> > > what will happen in the future..
> > > 
> > > So no objection too on having stats for dirty pages, it's just that if we
> > > still want to cover the overflow issue we'd make dirty_count writable, then
> > > it'd still better be in kvm_run, imho.
> > 
> > Yeah, never say never, but unless my math is wrong, overflow isn't happening anytime
> > soon.  And if future CPUs can overflow the number of dirty pages, then they'll be
> > able to overflow a number of stats, at which point I think we'll want a generic
> > ioctl() to reset _all_ stats.
> 
> It's slightly different as this affects functionality, unlike most stats.

Now that KVM exposes stats via ioctls, I don't think we should assume _anything_
about how userspace consumes stats.  I.e. KVM should guarantee sane behavior and
correctness for all stats.

In other words, this different in that it _directly_ affects KVM functionality,
but I think at this point we should assume that any and all stats can indirectly
affect KVM functionality.

> I'll leave that to Shivam to see his preference.  If to go that way, I hope
> we can at least have some WARN_ON_ONCE() on detecting overflows of "count".

I still think it's silly, but I definitely won't object to:

	WARN_ON_ONCE(!++vcpu->stat.generic.pages_dirtied);