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