Re: [PATCH 2/2] kvm: Add ioctl for gathering debug counters

[<milanpa@amazon.com>]

On 1/20/20 6:53 PM, Alexander Graf wrote:
>
>
> On 18.01.20 00:38, Paolo Bonzini wrote:
>> On 15/01/20 15:59, Alexander Graf wrote:
>>> On 15.01.20 15:43, milanpa@amazon.com wrote:
>>>>>> Let's expose new interface to userspace for garhering these
>>>>>> statistics with one ioctl.
>>>>>>
>>>>>> Userspace application can read counter description once using
>>>>>> KVM_GET_SUPPORTED_DEBUGFS_STAT and periodically invoke the
>>>>>> KVM_GET_DEBUGFS_VALUES to get value update.
>>>>>
>>>>> This is an interface that requires a lot of logic and buffers from
>>>>> user space to retrieve individual, explicit counters. What if I just
>>>>> wanted to monitor the number of exits on every user space exit?
>>>>
>>>> In case we want to cover such latency sensitive use cases solution b),
>>>> with mmap'ed structs you suggested, would be a way to go, IMO.
>>>>
>>>>> Also, we're suddenly making the debugfs names a full ABI, because
>>>>> through this interface we only identify the individual stats through
>>>>> their names. That means we can not remove stats or change their
>>>>> names, because people may rely on them, no? Thining about this again,
>>>>> maybe they already are an ABI because people rely on them in debugfs
>>>>> though?
>>
>> In theory not, in practice I have treated them as a kind of "soft" ABI:
>> if the meaning changes you should rename them, and removing them is
>> fine, but you shouldn't for example change the unit of measure (which is
>> not hard since they are all counters :) but perhaps you could have
>> nanoseconds vs TSC cycles in the future for some counters).
>>
>>>>> I see two alternatives to this approach here:
>>>>>
>>>>> a) ONE_REG
>>>>>
>>>>> We can just add a new DEBUG arch in ONE_REG and expose ONE_REG per VM
>>>>> as well (if we really have to?). That gives us explicit identifiers
>>>>> for each stat with an explicit path to introduce new ones with very
>>>>> unique identifiers.
>> ONE_REG would force us to define constants for each counter, and would
>> make it hard to retire them.  I don't like this.
>
> Why does it make it hard to retire them? We would just return -EINVAL 
> on retrieval, like we do for any other non-supported ONE_REG.
>
> It's the same as a file not existing in debugfs/statfs. Or an entry in 
> the array of this patch to disappear.
>
>>
>>>>> b) part of the mmap'ed vcpu struct
>>
>> Same here.  Even if we say the semantics of the struct would be exposed
>> to userspace via KVM_GET_SUPPORTED_DEBUGFS_STAT, someone might end up
>> getting this wrong and expecting a particular layout.  Milan's proposed
>> API has the big advantage of being hard to get wrong for userspace.  And
>> pushing the aggregation to userspace is not a huge chore, but it's still
>> a chore.
>>
>> So unless someone has a usecase for latency-sensitive monitoring I'd
>> prefer to keep it simple (usually these kind of stats can even make
>> sense if you gather them over relatively large period of time, because
>> then you'll probably use something else like tracepoints to actually
>> pinpoint what's going on).
>
> I tend to agree. Fetching them via an explicit call into the kernel is 
> definitely the safer route.
>
>>
>>>>> 2) vcpu counters
>>>>>
>>>>> Most of the counters count on vcpu granularity, but debugfs only
>>>>> gives us a full VM view. Whatever we do to improve the situation, we
>>>>> should definitely try to build something that allows us to get the
>>>>> counters per vcpu (as well).
>>>>>
>>>>> The main purpose of these counters is monitoring. It can be quite
>>>>> important to know that only a single vCPU is going wild, compared to
>>>>> all of them for example.
>>>>
>>>> I agree, exposing per vcpu counters can be useful. I guess it didn't
>>>> make much sense exposing them through debugfs so aggregation was done
>>>> in kernel. However if we chose to go with approach 1-b) mmap counters
>>>> struct in userspace, we could do this.
>>>
>>> The reason I dislike the debugfs/statfs approach is that it generates a
>>> completely separate permission and access paths to the stats. That's
>>> great for full system monitoring, but really bad when you have multiple
>>> individual tenants on a single host.
>>
>> I agree, anything in sysfs is complementary to vmfd/vcpufd access.
>
> Cool :).
>
> So we only need to agree on ONE_REG vs. this patch mostly?

What about extending KVM_GET_SUPPORTED_DEBUGFS_STAT with some additional 
information like the data type and unit? ONE_REG exposes additional 
semantics about data.

>
> Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879