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

[Marc Zyngier <maz@kernel.org>]

On Sun, 25 Dec 2022 16:50:04 +0000,
Shivam Kumar <shivam.kumar1@nutanix.com> wrote:
> 
> 
> 
> On 08/12/22 1:00 pm, Shivam Kumar wrote:
> > 
> > 
> > On 08/12/22 1:23 am, Sean Christopherson wrote:
> >> On Wed, Dec 07, 2022, Marc Zyngier wrote:
> >>> On Tue, 06 Dec 2022 06:22:45 +0000,
> >>> Shivam Kumar <shivam.kumar1@nutanix.com> wrote:
> >>> You need to define the granularity of the counter, and account for
> >>> each fault according to its mapping size. If an architecture has 16kB
> >>> as the base page size, a 32MB fault (the size of the smallest block
> >>> mapping) must bump the counter by 2048. That's the only way userspace
> >>> can figure out what is going on.
> >> 
> >> I don't think that's true for the dirty logging case.  IIUC, when a
> >> memslot is
> >> being dirty logged, KVM forces the memory to be mapped with
> >> PAGE_SIZE granularity,
> >> and that base PAGE_SIZE is fixed and known to userspace. 
> >> I.e. accuracy is naturally
> >> provided for this primary use case where accuracy really matters,
> >> and so this is
> >> effectively a documentation issue and not a functional issue.
> > 
> > So, does defining "count" as "the number of write permission faults"
> > help in addressing the documentation issue? My understanding too is
> > that for dirty logging, we will have uniform granularity.
> > 
> > Thanks.
> > 
> >> 
> >>> Without that, you may as well add a random number to the counter, it
> >>> won't be any worse.
> >> 
> >> The stat will be wildly inaccurate when dirty logging isn't
> >> enabled, but that doesn't
> >> necessarily make the stat useless, e.g. it might be useful as a
> >> very rough guage
> >> of which vCPUs are likely to be writing memory.  I do agree though
> >> that the value
> >> provided is questionable and/or highly speculative.
> >> 
> >>> [...]
> >>> 
> >>>>>>> If you introduce additional #ifdefery here, why are the additional
> >>>>>>> fields in the vcpu structure unconditional?
> >>>>>> 
> >>>>>> pages_dirtied can be a useful information even if dirty quota
> >>>>>> throttling is not used. So, I kept it unconditional based on
> >>>>>> feedback.
> >>>>> 
> >>>>> Useful for whom? This creates an ABI for all architectures, and this
> >>>>> needs buy-in from everyone. Personally, I think it is a pretty useless
> >>>>> stat.
> >>>> 
> >>>> When we started this patch series, it was a member of the kvm_run
> >>>> struct. I made this a stat based on the feedback I received from the
> >>>> reviews. If you think otherwise, I can move it back to where it was.
> >>> 
> >>> I'm certainly totally opposed to stats that don't have a clear use
> >>> case. People keep piling random stats that satisfy their pet usage,
> >>> and this only bloats the various structures for no overall benefit
> >>> other than "hey, it might be useful". This is death by a thousand cut.
> >> 
> >> I don't have a strong opinion on putting the counter into kvm_run
> >> as an "out"
> >> fields vs. making it a state.  I originally suggested making it a
> >> stat because
> >> KVM needs to capture the information somewhere, so why not make it
> >> a stat?  But
> >> I am definitely much more cavalier when it comes to adding stats,
> >> so I've no
> >> objection to dropping the stat side of things.
> > 
> > I'll be skeptical about making it a stat if we plan to allow the
> > userspace to reset it at will.
> > 
> > 
> > Thank you so much for the comments.
> > 
> > Thanks,
> > Shivam
> 
> Hi Marc,
> Hi Sean,
> 
> Please let me know if there's any further question or feedback.

My earlier comments still stand: the proposed API is not usable as a
general purpose memory-tracking API because it counts faults instead
of memory, making it inadequate except for the most trivial cases.
And I cannot believe you were serious when you mentioned that you were
happy to make that the API.

This requires some serious work, and this series is not yet near a
state where it could be merged.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.