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
