Xavier Simonart <[email protected]> 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 <[email protected]> wrote: > > Xavier Simonart <[email protected]> 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 <[email protected]> > > --- > > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
