Re: API/ABIs: Re: [RFC PATCH v2 0/2] Add a new acquire resource to query vcpu statistics

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.03.2023 11:28, Matias Ezequiel Vara Larsen wrote:
> On Fri, Mar 10, 2023 at 12:34:33PM +0100, Jan Beulich wrote:
>> On 10.03.2023 11:58, Matias Ezequiel Vara Larsen wrote:
>>> Oh, I see, thanks for the clarification. To summarise, these are the current
>>> options:
>>> 1. Use a "uint64_t" field thus limiting the number of counters to 64. The
>>> current vcpu_runstate_info structure is limited to 4 counters though, one 
>>> for
>>> each RUNSTATE_*. 
>>> 2. Use a dynamic array but this makes harder to use the interface.
>>> 3. Eliminate stats_active and set to ~0 the actual stats value to mark 
>>> inactive
>>> counters. This requires adding a "nr_stats" field to know how many counters 
>>> are.
>>
>> While nr_stats can indeed be seen as a generalization of the earlier
>> stats_active, I think it is possible to get away without, as long as
>> padding fields also are filled with the "inactive" marker.
>>
> 
> Understood.
> 
>>> Also, this requires to make sure to saturate at 2^^64-2.
>>
>> Thinking of it - considering overflowed counters inactive looks like a
>> reasonable model to me as well (which would mean saturating at 2^^64-1).
>>
>>> I might miss some details here but these are the options to evaluate. 
>>>
>>> I would go with a variation of 1) by using two uint64_t, i.e., up to 128 
>>> vcpu's
>>> counters, which I think it would be enough. I may be wrong.
>>
>> Well, to me it doesn't matter whether it's 32, 64, or 128 - my concern
>> is with any kind of inherent upper bound. Using 128 right away might
>> look excessive, just like 32 might look too little. Hence my desire to
>> get away without any built-in upper bound. IOW I continue to favor 3,
>> irrespective of the presence or absence of nr_stats.
>>
> I see. 3) layout would look like:
> 
> struct vcpu_shmem_stats {
> #define VCPU_STATS_MAGIC 0xaabbccdd
>     uint32_t magic;
>     uint32_t offset;  // roundup(sizeof(vcpu_shmem_stats), cacheline_size)
>     uint32_t size;    // sizeof(vcpu_stats)
>     uint32_t stride;  // roundup(sizeof(vcpu_stats), cacheline_size)
> };
> 
> struct vcpu_stats {
>     /*
>      * If the least-significant bit of the seq number is set then an update
>      * is in progress and the consumer must wait to read a consistent set of
>      * values. This mechanism is similar to Linux's seqlock.
>      */
>     uint32_t seq;
>     uint32 _pad;
>     /*
>      * If the most-significant bit of a counter is set then the counter
>      * is inactive and the consumer must ignore its value. Note that this
>      * could also indicate that the counter has overflowed.
>      */
>     uint64_t stats_a; // e.g., runstate_running_time
>     ...
> };
> 
> All padding fields shall be marked as "inactive". The consumer can't
> distinguish inactive from overflowed. Also, the consumer shall always verify
> before reading that:
> 
> offsetof(struct vcpu_stats, stats_y) < size. 
> 
> in case the consumer knows about a counter, e.g., stats_y, that Xen does not
> it.

Looks okay to me this way, but please have this verified by another party
(preferably Andrew, whose proposal was now changed).

Jan