Re: [PATCH v1 1/4] KVM: stats: Support linear and logarithmic histogram statistics

[Jing Zhang <jingzhangos@google.com>]

On Wed, Jul 28, 2021 at 5:39 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 06/07/21 20:03, Jing Zhang wrote:
> > +#define LINHIST_SIZE_SMALL           10
> > +#define LINHIST_SIZE_MEDIUM          20
> > +#define LINHIST_SIZE_LARGE           50
> > +#define LINHIST_SIZE_XLARGE          100
> > +#define LINHIST_BUCKET_SIZE_SMALL    10
> > +#define LINHIST_BUCKET_SIZE_MEDIUM   100
> > +#define LINHIST_BUCKET_SIZE_LARGE    1000
> > +#define LINHIST_BUCKET_SIZE_XLARGE   10000
> > +
> > +#define LOGHIST_SIZE_SMALL           8
> > +#define LOGHIST_SIZE_MEDIUM          16
> > +#define LOGHIST_SIZE_LARGE           32
> > +#define LOGHIST_SIZE_XLARGE          64
> > +#define LOGHIST_BASE_2                       2
>
> I'd prefer inlining all of these.  For log histograms use 2 directly in
> STATS_DESC_LOG_HIST, since the update function below uses fls64.
>
Sure, will inline these values.
Will remove the loghist base, since base 2 log is enough for any
number. No other base is needed.
> >
> > + */
> > +void kvm_stats_linear_hist_update(u64 *data, size_t size,
> > +                               u64 value, size_t bucket_size)
> > +{
> > +     size_t index = value / bucket_size;
> > +
> > +     if (index >= size)
> > +             index = size - 1;
> > +     ++data[index];
> > +}
> > +
>
> Please make this function always inline, so that the compiler optimizes
> the division.
Sure.
>
> Also please use array_index_nospec to clamp the index to the size, in
> case value comes from a memory access as well.  Likewise for
> kvm_stats_log_hist_update.
Thanks. Will do.
>
> Paolo
>

Jing