[ovs-dev] [subfacet 1/4] ofproto-dpif: Simplify subfacet stat calculations.

[Ben Pfaff]

On Wed, Jun 05, 2013 at 01:46:18PM -0700, Ethan Jackson wrote:
> Along with some minor minor cleanups, this patch simplifies the
> calculation of the average subfacet count, and average subfacet
> lifespan.  They are both calculated as exponentially weighted
> moving averages updated immediately after statistics are pulled
> from the datapath.  This both simplifies the code, and makes the
> statistics produced more useful.  Specifically, with the average
> total subfacet lifespan, subfacets which are still in the datapath
> now count towards that average.
> 
> Signed-off-by: Ethan Jackson <ethan at nicira.com>

The commit message still sounds a lot like this is just a refactoring.
It would be nice to be clear that it changes the meaning and
interpretation of the statistics, not just the form of calculation.

> @@ -4305,15 +4291,30 @@ expire(struct dpif_backer *backer)
>      update_stats(backer);
>  
>      HMAP_FOR_EACH (ofproto, all_ofproto_dpifs_node, &all_ofproto_dpifs) {
> +        long long int avg_subfacet_life_span;
>          struct rule *rule, *next_rule;
> +        struct subfacet *subfacet;
>          int dp_max_idle;
>  
>          if (ofproto->backer != backer) {
>              continue;
>          }
>  
> -        /* Keep track of the max number of flows per ofproto_dpif. */
> -        update_max_subfacet_count(ofproto);
> +        avg_subfacet_life_span = 0;
> +        if (!hmap_is_empty(&ofproto->subfacets)) {
> +            long long int now = time_msec();
> +            HMAP_FOR_EACH (subfacet, hmap_node, &ofproto->subfacets) {
> +                avg_subfacet_life_span += now - subfacet->created;
> +            }
> +            avg_subfacet_life_span /= hmap_count(&ofproto->subfacets);
> +        }
> +        ofproto->avg_subfacet_life_span =
> +            (ofproto->avg_subfacet_life_span + avg_subfacet_life_span) / 2;
> +
> +        ofproto->avg_n_subfacet += hmap_count(&ofproto->subfacets);
> +        ofproto->avg_n_subfacet /= 2;
> +        ofproto->max_n_subfacet = MAX(ofproto->max_n_subfacet,
> +                                      hmap_count(&ofproto->subfacets));

Two EWMAs are calculated here in two different ways, right next to
each other:

     a = (a + b) / 2;

     c += d;
     c /= 2;

The style contrast is jarring, please pick one style (I like the
second one a little better since the names are so long).

> @@ -8322,12 +8318,12 @@ show_dp_format(const struct ofproto_dpif *ofproto, struct ds *ds)
>      ds_put_format(ds,
>                    "\tlookups: hit:%"PRIu64" missed:%"PRIu64"\n",
>                    ofproto->n_hit, ofproto->n_missed);
> -    ds_put_format(ds, "\tflows: cur: %zu, avg: %5.3f, max: %d,"
> +    ds_put_format(ds, "\tflows: cur: %zu, avg: %d, max: %d,"
>                    " life span: %llu(ms)\n",
>                    hmap_count(&ofproto->subfacets),
> -                  avg_subfacet_count(ofproto),
> +                  ofproto->avg_n_subfacet,

I think that avg_n_subfacet is an unsigned int?  If so please use %u
instead of %d.
>                    ofproto->max_n_subfacet,
> -                  avg_subfacet_life_span(ofproto));
> +                  ofproto->avg_subfacet_life_span);

Similarly, I think that avg_subfacet_life_span is signed, so please
use %lld instead of %llu.