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