[ovs-dev] [PATCH ovs] lib: export calc_percentile function

Aaron Conole aconole at redhat.com
Thu Oct 14 19:39:40 UTC 2021


Xavier Simonart <xsimonar at redhat.com> writes:

> Hi Aaron
>
> Thanks for looking into this.
>
> You are right when saying that OVN uses the stopwatch API just fine.
> The goal of the proposed modification is to be able provide extra counters/statistics from OVN.
> For instance, there was some interest about how many flows (or groups) are added (removed, updated, ...) by
> ovn-controller.
> In addition to the raw count, we'd like to provide some statistical view per iteration - hence the need of being able to
> calculate things like percentiles (in addition to average, max, ...).
>
> The stopwatch API provides most of what would be needed, but would have required some changes:
> - stopwatch always uses some "units" (msec, ...) while we would use it here to report non time-based statistics => we
> would need to add some other "units". We could add an "empty" unit, to avoid having to require OVS changes
> everytime we need different statistics (today flows, tomorrow something else).
> - more important, stopwatch does not provide the total count we are looking for (i.e. the sum of all samples). We could
> add a "Total" or a "Sum" to stopwatch, but this would have changed the stopwatch outputs for all existing counters. I
> felt that this might be perceived as an API change or a compatibility issue.
>
> Hence I was proposing to only export the percentile related function, to avoid any change to OVS which might impact
> backward compatibility.
> But I would be happy to modify the stopwatch, adding the "Sum" and the empty unit. This would simplify OVN code as
> well compared to what I was initially proposing.
> Adding stopwatch_add_sample(...) would not change much in this case.

I see - what you're looking for is some kind of generic statistics -
would the coverage counters work instead?  We do keep some flow
statistics, so I guess it's useful to understand what you're looking
to track.  Maybe 'stopwatch' might not work perfectly, or maybe we can
make some changes to make it seem more generic.

> Thanks
> Xavier
>
> On Wed, Oct 13, 2021 at 6:42 PM Aaron Conole <aconole at redhat.com> wrote:
>
>  Xavier Simonart <xsimonar at redhat.com> writes:
>
>  > export calc_percentile function (and percentile struct) so that
>  > it can be used in other libraries such as OVN.
>  >
>  > Signed-off-by: Xavier Simonart <xsimonar at redhat.com>
>  > ---
>
>  What is the intent to use this in other libraries?  It would be nice to
>  understand why just running the existing stopwatch API doesn't work
>  (maybe you cannot start samples).  AFAIK, OVN uses the stopwatch API
>  just fine?
>
>  Perhaps it could make sense to expose the functionality in a different
>  fashion like:
>
>    void stopwatch_add_sample(const char *, unsigned long long);
>
>  This seems a useful API and doesn't expose all of the internal
>  information, but I don't know if it really is needed.  Can you expand
>  why you need calc_percentile exposed?
>
>  >  lib/stopwatch.c | 24 +-----------------------
>  >  lib/stopwatch.h | 26 ++++++++++++++++++++++++++
>  >  2 files changed, 27 insertions(+), 23 deletions(-)
>  >
>  > diff --git a/lib/stopwatch.c b/lib/stopwatch.c
>  > index f5602163b..003c3a05f 100644
>  > --- a/lib/stopwatch.c
>  > +++ b/lib/stopwatch.c
>  > @@ -35,25 +35,6 @@ struct average {
>  >      double alpha;   /* Weight given to new samples */
>  >  };
>  >  
>  > -#define MARKERS 5
>  > -
>  > -/* Number of samples to collect before reporting P-square calculated
>  > - * percentile
>  > - */
>  > -#define P_SQUARE_MIN 50
>  > -
>  > -/* The naming of these fields is based on the naming used in the
>  > - * P-square algorithm paper.
>  > - */
>  > -struct percentile {
>  > -    int n[MARKERS];
>  > -    double n_prime[MARKERS];
>  > -    double q[MARKERS];
>  > -    double dn[MARKERS];
>  > -    unsigned long long samples[P_SQUARE_MIN];
>  > -    double percentile;
>  > -};
>  > -
>  >  struct stopwatch {
>  >      enum stopwatch_units units;
>  >      unsigned long long n_samples;
>  > @@ -107,10 +88,7 @@ comp_samples(const void *left, const void *right)
>  >      return *right_d > *left_d ? -1 : *right_d < *left_d;
>  >  }
>  >  
>  > -/* Calculate the percentile using the P-square algorithm. For more
>  > - * information, see https://www1.cse.wustl.edu/~jain/papers/ftp/psqr.pdf
>  > - */
>  > -static void
>  > +void
>  >  calc_percentile(unsigned long long n_samples, struct percentile *pctl,
>  >                  unsigned long long new_sample)
>  >  {
>  > diff --git a/lib/stopwatch.h b/lib/stopwatch.h
>  > index 91abd64e4..efb9a9e8a 100644
>  > --- a/lib/stopwatch.h
>  > +++ b/lib/stopwatch.h
>  > @@ -36,6 +36,32 @@ struct stopwatch_stats {
>  >                                      (alpha 0.01). */
>  >  };
>  >  
>  > +#define MARKERS 5
>  > +
>  > +/* Number of samples to collect before reporting P-square calculated
>  > + * percentile
>  > + */
>  > +#define P_SQUARE_MIN 50
>  > +
>  > +/* The naming of these fields is based on the naming used in the
>  > + * P-square algorithm paper.
>  > + */
>  > +struct percentile {
>  > +    int n[MARKERS];
>  > +    double n_prime[MARKERS];
>  > +    double q[MARKERS];
>  > +    double dn[MARKERS];
>  > +    unsigned long long samples[P_SQUARE_MIN];
>  > +    double percentile;
>  > +};
>  > +
>  > +/* Calculate the percentile using the P-square algorithm. For more
>  > + * information, see https://www1.cse.wustl.edu/~jain/papers/ftp/psqr.pdf
>  > + */
>  > +void
>  > +calc_percentile(unsigned long long n_samples, struct percentile *pctl,
>  > +                unsigned long long new_sample);
>  > +
>  >  /* Create a new stopwatch.
>  >   * The "units" are not used for any calculations but are printed when
>  >   * statistics are requested.



More information about the dev mailing list