Re: [PATCH v3 0/2] KVM: s390: add counters for vsie performance

[David Hildenbrand <david@redhat.com>]

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.

-- 
Cheers,

David / dhildenb