On Sun, 25 Dec 2022 16:50:04 +0000, Shivam Kumar <shivam.kumar1@xxxxxxxxxxx> 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@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 > > 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.
