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

[Sean Christopherson <seanjc@google.com>]

On Thu, May 26, 2022, Marc Zyngier wrote:
> On Thu, 26 May 2022 16:44:13 +0100,
> Sean Christopherson <seanjc@google.com> wrote:
> > 
> > On Thu, May 26, 2022, Marc Zyngier wrote:
> > > > >> +{
> > > > >> +	struct kvm_run *run = vcpu->run;
> > > > >> +	u64 dirty_quota = READ_ONCE(run->dirty_quota);
> > > > >> +	u64 pages_dirtied = vcpu->stat.generic.pages_dirtied;
> > > > >> +
> > > > >> +	if (!dirty_quota || (pages_dirtied < dirty_quota))
> > > > >> +		return 1;
> > > > > What happens when page_dirtied becomes large and dirty_quota has to
> > > > > wrap to allow further progress?
> > > > Every time the quota is exhausted, userspace is expected to set it to
> > > > pages_dirtied + new quota. So, pages_dirtied will always follow dirty
> > > > quota. I'll be sending the qemu patches soon. Thanks.
> > > 
> > > Right, so let's assume that page_dirtied=0xffffffffffffffff (yes, I
> > > have dirtied that many pages).
> > 
> > Really?  Written that many bytes from a guest?  Maybe.  But actually
> > marked that many pages dirty in hardware, let alone in KVM?  And on
> > a single CPU?
> >
> > By my back of the napkin math, a 4096 CPU system running at 16ghz
> > with each CPU able to access one page of memory per cycle would take
> > ~3 days to access 2^64 pages.
> > 
> > Assuming a ridiculously optimistic ~20 cycles to walk page tables,
> > fetch the cache line from memory, insert into the TLB, and mark the
> > PTE dirty, that's still ~60 days to actually dirty that many pages
> > in hardware.
> > 
> > Let's again be comically optimistic and assume KVM can somehow
> > propagate a dirty bit from hardware PTEs to the dirty bitmap/ring in
> > another ~20 cycles.  That brings us to ~1200 days.
> > 
> > But the stat is per vCPU, so that actually means it would take
> > ~13.8k years for a single vCPU/CPU to dirty 2^64 pages... running at
> > a ludicrous 16ghz on a CPU with latencies that are a likely an order
> > of magnitude faster than anything that exists today.
> 
> Congratulations, you can multiply! ;-)

Don't ask me how long it took for me do to that math :-D

> It just shows that the proposed API is pretty bad, because instead of
> working as a credit, it works as a ceiling, based on a value that is
> dependent on the vpcu previous state (forcing userspace to recompute
> the next quota on each exit),

My understanding is that intended use case is dependent on previous vCPU state by
design.  E.g. a vCPU that is aggressively dirtying memory will be given a different
quota than a vCPU that has historically not dirtied much memory.

Even if the quotas are fixed, "recomputing" the new quota is simply:

	run->dirty_quota = run->dirty_quota_exit.count + PER_VCPU_DIRTY_QUOTA

The main motivation for the ceiling approach is that it's simpler for KVM to implement
since KVM never writes vcpu->run->dirty_quota.  E.g. userspace may observe a "spurious"
exit if KVM reads the quota right before it's changed by userspace, but KVM doesn't
have to guard against clobbering the quota.

A credit based implementation would require (a) snapshotting the credit at
some point during of KVM_RUN, (b) disallowing changing the quota credit while the
vCPU is running, or (c) an expensive atomic sequence (expensive on x86 at least)
to avoid clobbering an update from userspace.  (a) is undesirable because it delays
recognition of the new quota, especially if KVM doesn't re-read the quota in the
tight run loop.  And AIUI, changing the quota while the vCPU is running is a desired
use case, so that rules out (b).  The cost of (c) is not the end of the world, but
IMO the benefits are marginal.

E.g. if we go with a request-based implementation where kvm_vcpu_check_dirty_quota()
is called in mark_page_dirty_in_slot(), then the ceiling-based approach is:

  static void kvm_vcpu_is_dirty_quota_exhausted(struct kvm_vcpu *vcpu)
  {
        u64 dirty_quota = READ_ONCE(vcpu->run->dirty_quota);

	if (!dirty_quota || (vcpu->stat.generic.pages_dirtied < dirty_quota))
		return;

	/*
	 * Snapshot the quota to report it to userspace.  The dirty count will
	 * captured when the request is processed.
	 */
	vcpu->dirty_quota = dirty_quota;
	kvm_make_request(KVM_REQ_DIRTY_QUOTA_EXIT, vcpu);
  }

whereas the credit-based approach would need to be something like:

  static void kvm_vcpu_is_dirty_quota_exhausted(struct kvm_vcpu *vcpu)
  {
        u64 old_dirty_quota;

	if (!READ_ONCE(vcpu->run->dirty_quota_enabled))
		return;

	old_dirty_quota = atomic64_fetch_add_unless(vcpu->run->dirty_quota, -1, 0);

	/* Quota is not yet exhausted, or was already exhausted. */
	if (old_dirty_quota != 1)
		return;

	/*
	 * The dirty count will be re-captured in dirty_count when the request
	 * is processed so that userspace can compute the number of pages that
	 * were dirtied after the quota was exhausted.
	 */
	vcpu->dirty_count_at_exhaustion = vcpu->stat.generic.pages_dirtied;
	kvm_make_request(KVM_REQ_DIRTY_QUOTA_EXIT, vcpu);
  }

> and with undocumented, arbitrary limits as a bonus.

Eh, documenting that userspace needs to be aware of a theoretically-possible wrap
is easy enough.  If we're truly worried about a wrap scenario, it'd be trivial to
add a flag/ioctl() to let userspace reset vcpu->stat.generic.pages_dirtied.