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

[Matias Ezequiel Vara Larsen <matiasevara@xxxxxxxxx>]

On Wed, Mar 08, 2023 at 03:16:05PM +0100, Jan Beulich wrote:
> On 08.03.2023 12:54, Matias Ezequiel Vara Larsen wrote:
> > On Tue, Mar 07, 2023 at 11:12:00AM +0100, Jan Beulich wrote:
> >> On 06.03.2023 15:23, Matias Ezequiel Vara Larsen wrote:
> >>> - Xen shall use the "stats_active" field to indicate what it is 
> >>> producing. In
> >>>   this field, reserved bits shall be 0. This shall allow us to agree on 
> >>> the
> >>> layout even when producer and consumer are compiled with different 
> >>> headers.
> >>
> >> I wonder how well such a bitfield is going to scale. It provides for
> >> only 32 (maybe 64) counters. Of course this may seem a lot right now,
> >> but you never know how quickly something like this can grow. Plus
> >> with ...
> >>
> > 
> > Would it make sense to define it like this?:
> > 
> > struct vcpu_shmem_stats {
> > #define STATS_A (1u << 0)
> > ...
> > #define VCPU_STATS_MAGIC 0xaabbccdd
> >      uint32_t magic;
> >      uint32_t offset;  // roundup(sizeof(vcpu_shmem_stats) + 
> > sizeof(uint32_t) * nr_stats, cacheline_size)
> >      uint32_t size;    // sizeof(vcpu_stats)
> >      uint32_t stride;  // roundup(sizeof(vcpu_stats), cacheline_size)
> >      uint32_t nr_stats; // size of stats_active in uint32_t
> >      uint32_t stats_active[XEN_FLEX_ARRAY_DIM];
> >      ...
> > };
> 

The use of stats_active[] is meant to have a bitmap that could scale thus not
limiting the number of counters in the vcpu_stat structure to 32 or 64. I can't
see other way to have an unlimited number of counters though.

> Possibly, but this would make it harder to use the interface. An alternative
> might be to specify that an actual stats value set to ~0 marks an inactive
> element. Since these are 64-bit counters, with today's and tomorrow's
> computers we won't be at risk of a counter reaching a value of 2^^64-1, I
> guess. And even if there was one where such a risk could not be excluded
> (e.g. because of using pretty large increments), one would merely need to
> make sure to saturate at 2^^64-2. Plus at such a point one would need to
> consider anyway to switch to 128-bit fields, as neither saturated nor
> wrapped values are of much use to consumers.
> 

If I understand well, this use-case is in case an element in the stats_active
bitmap becomes inactive, i.e., it is set to "0" in stats_active[]. You are
proposing to set to ~0 the actual stats value to mark an inactive element. I
may be missing something here but would not be enough to set to "0" the
corresponding stats_active[] bit? 

> >>> - In the vcpu_stats structure, new fields can only ever be appended.
> >>
> >> ... this rule the only ambiguity that could arise to consumers when
> >> no active flags existed would be that they can't tell "event never
> >> occurred" from "hypervisor doesn't know about this anymore".
> > 
> > Do you mean how the consumer can figure out if either 1) Xen does not know 
> > yet
> > about some flag or 2) the flag has been deprecated? I think 2) is the case 
> > that
> > Andrew mention in which the magic number could be used as an ABI version to
> > break backwards compatibility.
> 
> No, an inactive field wouldn't break the ABI. An ABI change would be if
> such an inactive field was actually removed from the array. Or if e.g.,
> as per above, we needed to switch to 128-bit counters.

Got it, Thanks.

Matias