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.
Matias
