| Message ID | 20230904130140.22006-1-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: s390: add counters for vsie performance | expand |
On Mon, 2023-09-04 at 15:01 +0200, Nico Boehr wrote: > v3: > --- > * rename te -> entry (David) > * add counters for gmap reuse and gmap create (David) > > v2: > --- > * also count shadowing of pages (Janosch) > * fix naming of counters (Janosch) > * mention shadowing of multiple levels is counted in each level (Claudio) > * fix inaccuate commit description regarding gmap notifier (Claudio) > > When running a guest-3 via VSIE, guest-1 needs to shadow the page table > structures of guest-2. > > To reflect changes of the guest-2 in the _shadowed_ page table structures, > the _shadowing_ sturctures sometimes need to be rebuilt. Since this is a > costly operation, it should be avoided whenever possible. > > This series adds kvm stat counters to count the number of shadow gmaps > created and a tracepoint whenever something is unshadowed. This is a first > step to try and improve VSIE performance. > > Please note that "KVM: s390: add tracepoint in gmap notifier" has some > checkpatch --strict findings. I did not fix these since the tracepoint > definition would then look completely different from all the other > tracepoints in arch/s390/kvm/trace-s390.h. If you want me to fix that, > please let me know. > > While developing this, a question regarding the stat counters came up: > there's usually no locking involved when the stat counters are incremented. > On s390, GCC accidentally seems to do the right thing(TM) most of the time > by generating a agsi instruction (which should be atomic given proper > alignment). However, it's not guaranteed, so would we rather want to go > with an atomic for the stat counters to avoid losing events? Or do we just > accept the fact that we might loose events sometimes? Is there anything > that speaks against having an atomic in kvm_stat? > FWIW the PCI counters (/sys/kernel/debug/pci/<dev>/statistics) use atomic64_add(). Also, s390's memory model is strong enough that these are actually just normal loads/stores/adds (see arch/s390/include/asm/atomic_ops.h) with the generic atomic64_xx() adding debug instrumentation.
Quoting Niklas Schnelle (2023-09-05 09:53:40) > On Mon, 2023-09-04 at 15:01 +0200, Nico Boehr wrote: > > v3: > > --- > > * rename te -> entry (David) > > * add counters for gmap reuse and gmap create (David) > > > > v2: > > --- > > * also count shadowing of pages (Janosch) > > * fix naming of counters (Janosch) > > * mention shadowing of multiple levels is counted in each level (Claudio) > > * fix inaccuate commit description regarding gmap notifier (Claudio) > > > > When running a guest-3 via VSIE, guest-1 needs to shadow the page table > > structures of guest-2. > > > > To reflect changes of the guest-2 in the _shadowed_ page table structures, > > the _shadowing_ sturctures sometimes need to be rebuilt. Since this is a > > costly operation, it should be avoided whenever possible. > > > > This series adds kvm stat counters to count the number of shadow gmaps > > created and a tracepoint whenever something is unshadowed. This is a first > > step to try and improve VSIE performance. > > > > Please note that "KVM: s390: add tracepoint in gmap notifier" has some > > checkpatch --strict findings. I did not fix these since the tracepoint > > definition would then look completely different from all the other > > tracepoints in arch/s390/kvm/trace-s390.h. If you want me to fix that, > > please let me know. > > > > While developing this, a question regarding the stat counters came up: > > there's usually no locking involved when the stat counters are incremented. > > On s390, GCC accidentally seems to do the right thing(TM) most of the time > > by generating a agsi instruction (which should be atomic given proper > > alignment). However, it's not guaranteed, so would we rather want to go > > with an atomic for the stat counters to avoid losing events? Or do we just > > accept the fact that we might loose events sometimes? Is there anything > > that speaks against having an atomic in kvm_stat? > > > > FWIW the PCI counters (/sys/kernel/debug/pci/<dev>/statistics) use > atomic64_add(). Also, s390's memory model is strong enough that these > are actually just normal loads/stores/adds (see > arch/s390/include/asm/atomic_ops.h) with the generic atomic64_xx() > adding debug instrumentation. In KVM I am mostly concerned about the compiler since we just do counter++ - right now this always seems to result in an agsi instruction, but that's of course not guaranteed.
On 05.09.23 10:33, Nico Boehr wrote: > Quoting Niklas Schnelle (2023-09-05 09:53:40) >> On Mon, 2023-09-04 at 15:01 +0200, Nico Boehr wrote: >>> v3: >>> --- >>> * rename te -> entry (David) >>> * add counters for gmap reuse and gmap create (David) >>> >>> v2: >>> --- >>> * also count shadowing of pages (Janosch) >>> * fix naming of counters (Janosch) >>> * mention shadowing of multiple levels is counted in each level (Claudio) >>> * fix inaccuate commit description regarding gmap notifier (Claudio) >>> >>> When running a guest-3 via VSIE, guest-1 needs to shadow the page table >>> structures of guest-2. >>> >>> To reflect changes of the guest-2 in the _shadowed_ page table structures, >>> the _shadowing_ sturctures sometimes need to be rebuilt. Since this is a >>> costly operation, it should be avoided whenever possible. >>> >>> This series adds kvm stat counters to count the number of shadow gmaps >>> created and a tracepoint whenever something is unshadowed. This is a first >>> step to try and improve VSIE performance. >>> >>> Please note that "KVM: s390: add tracepoint in gmap notifier" has some >>> checkpatch --strict findings. I did not fix these since the tracepoint >>> definition would then look completely different from all the other >>> tracepoints in arch/s390/kvm/trace-s390.h. If you want me to fix that, >>> please let me know. >>> >>> While developing this, a question regarding the stat counters came up: >>> there's usually no locking involved when the stat counters are incremented. >>> On s390, GCC accidentally seems to do the right thing(TM) most of the time >>> by generating a agsi instruction (which should be atomic given proper >>> alignment). However, it's not guaranteed, so would we rather want to go >>> with an atomic for the stat counters to avoid losing events? Or do we just >>> accept the fact that we might loose events sometimes? Is there anything >>> that speaks against having an atomic in kvm_stat? >>> >> >> FWIW the PCI counters (/sys/kernel/debug/pci/<dev>/statistics) use >> atomic64_add(). Also, s390's memory model is strong enough that these >> are actually just normal loads/stores/adds (see >> arch/s390/include/asm/atomic_ops.h) with the generic atomic64_xx() >> adding debug instrumentation. > > In KVM I am mostly concerned about the compiler since we just do counter++ > - right now this always seems to result in an agsi instruction, but that's > of course not guaranteed. Right, the compiler can do what it wants with that. The question is if we care about a slight imprecision, though. Probably not worth the trouble for something that never happens and is only used for debugging purposes. Using atomics would be cleaner, though.
Quoting David Hildenbrand (2023-09-06 09:37:28) [...] > Right, the compiler can do what it wants with that. The question is if > we care about a slight imprecision, though. > > Probably not worth the trouble for something that never happens and is > only used for debugging purposes. Yep, probably true. Thanks!