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 Thu, Mar 09, 2023 at 12:50:18PM +0100, Jan Beulich wrote:
> On 09.03.2023 11:38, Matias Ezequiel Vara Larsen wrote:
> > 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? 
> 
> The suggestion was to eliminate the need for stats_active[].
> 
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.
Also, this requires to make sure to saturate at 2^^64-2.

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.

Thoughs?
 
Matias