[ovs-dev] [PATCH 1/2] ofproto-dpif: implement group and group bucket stats

Andy Zhou azhou at nicira.com
Wed Apr 9 23:48:04 UTC 2014


Forgot to add  marco.canini at acm.org to CC

On Wed, Apr 9, 2014 at 4:43 PM, Andy Zhou <azhou at nicira.com> wrote:
> Fix a bug in Openflow group implementation that neither group stats
> nor per bucket stats are properly updated.
>
> Reported-by: Marco Canini <marco.canini at acm.org>
> Signed-off-by: Andy Zhou <azhou at nicira.com>
> ---
>  AUTHORS                      |    1 +
>  ofproto/ofproto-dpif-xlate.c |   22 ++++++++++++++++------
>  ofproto/ofproto-dpif-xlate.h |    2 +-
>  ofproto/ofproto-dpif.c       |   39 +++++++++++++++++++++++++++++++++++++++
>  ofproto/ofproto-dpif.h       |    3 +++
>  5 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 78640b8..e83af71 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -212,6 +212,7 @@ Len Gao                 leng at vmware.com
>  Logan Rosen             logatronico at gmail.com
>  Luca Falavigna          dktrkranz at debian.org
>  Luiz Henrique Ozaki     luiz.ozaki at gmail.com
> +Marco Canini            marco.canini at acm.org
>  Marco d'Itri            md at Linux.IT
>  Maxime Brun             m.brun at alphalink.fr
>  Michael A. Collins      mike.a.collins at ark-net.org
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 7b6e9f7..5dd4f93 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -765,20 +765,21 @@ odp_port_is_alive(const struct xlate_ctx *ctx, ofp_port_t ofp_port)
>
>  static const struct ofputil_bucket *
>  group_first_live_bucket(const struct xlate_ctx *, const struct group_dpif *,
> -                        int depth);
> +                        int depth, int *index);
>
>  static bool
>  group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth)
>  {
>      struct group_dpif *group;
>      bool hit;
> +    int unused;
>
>      hit = group_dpif_lookup(ctx->xbridge->ofproto, group_id, &group);
>      if (!hit) {
>          return false;
>      }
>
> -    hit = group_first_live_bucket(ctx, group, depth) != NULL;
> +    hit = group_first_live_bucket(ctx, group, depth, &unused) != NULL;
>
>      group_dpif_release(group);
>      return hit;
> @@ -807,16 +808,19 @@ bucket_is_alive(const struct xlate_ctx *ctx,
>
>  static const struct ofputil_bucket *
>  group_first_live_bucket(const struct xlate_ctx *ctx,
> -                        const struct group_dpif *group, int depth)
> +                        const struct group_dpif *group, int depth, int *index)
>  {
>      struct ofputil_bucket *bucket;
>      const struct list *buckets;
> +    int i = 0;
>
>      group_dpif_get_buckets(group, &buckets);
>      LIST_FOR_EACH (bucket, list_node, buckets) {
>          if (bucket_is_alive(ctx, bucket, depth)) {
> +            *index = i;
>              return bucket;
>          }
> +        i++;
>      }
>
>      return NULL;
> @@ -825,7 +829,7 @@ group_first_live_bucket(const struct xlate_ctx *ctx,
>  static const struct ofputil_bucket *
>  group_best_live_bucket(const struct xlate_ctx *ctx,
>                         const struct group_dpif *group,
> -                       uint32_t basis)
> +                       uint32_t basis, int *index)
>  {
>      const struct ofputil_bucket *best_bucket = NULL;
>      uint32_t best_score = 0;
> @@ -841,6 +845,7 @@ group_best_live_bucket(const struct xlate_ctx *ctx,
>              if (score >= best_score) {
>                  best_bucket = bucket;
>                  best_score = score;
> +                *index = i;
>              }
>          }
>          i++;
> @@ -2022,16 +2027,19 @@ xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group)
>           * just before applying the all or indirect group. */
>          ctx->xin->flow = old_flow;
>      }
> +    group_dpif_credit_stats(group, -1, ctx->xin->resubmit_stats);
>  }
>
>  static void
>  xlate_ff_group(struct xlate_ctx *ctx, struct group_dpif *group)
>  {
>      const struct ofputil_bucket *bucket;
> +    int index;
>
> -    bucket = group_first_live_bucket(ctx, group, 0);
> +    bucket = group_first_live_bucket(ctx, group, 0, &index);
>      if (bucket) {
>          xlate_group_bucket(ctx, bucket);
> +        group_dpif_credit_stats(group, index, ctx->xin->resubmit_stats);
>      }
>  }
>
> @@ -2041,12 +2049,14 @@ xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
>      struct flow_wildcards *wc = &ctx->xout->wc;
>      const struct ofputil_bucket *bucket;
>      uint32_t basis;
> +    int index;
>
>      basis = hash_mac(ctx->xin->flow.dl_dst, 0, 0);
> -    bucket = group_best_live_bucket(ctx, group, basis);
> +    bucket = group_best_live_bucket(ctx, group, basis, &index);
>      if (bucket) {
>          memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
>          xlate_group_bucket(ctx, bucket);
> +        group_dpif_credit_stats(group, index, ctx->xin->resubmit_stats);
>      }
>  }
>
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index 8b53e10..bb3bad0 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -122,7 +122,7 @@ struct xlate_in {
>      void (*report_hook)(struct xlate_in *, const char *s, int recurse);
>
>      /* If nonnull, flow translation credits the specified statistics to each
> -     * rule reached through a resubmit or OFPP_TABLE action.
> +     * rule reached through a resubmit, OFPP_TABLE or group action.
>       *
>       * This is normally null so the client has to set it manually after
>       * calling xlate_in_init(). */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index cc36a0f..eb510c6 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3623,6 +3623,45 @@ group_dpif_get_type(const struct group_dpif *group)
>  {
>      return group->up.type;
>  }
> +
> +/* Credit flow stats to group. In addition, if 'bucket_index' is a negative
> + * value, credit the stats to all buckets. Otherwise credit the stats to
> + * a single bucket 'bucket_index' refers to.  */
> +void
> +group_dpif_credit_stats(struct group_dpif *group, int bucket_index,
> +                        const struct dpif_flow_stats *stats)
> +{
> +    if (!stats) {
> +        return;
> +    }
> +
> +    ovs_mutex_lock(&group->stats_mutex);
> +
> +    group->packet_count += stats->n_packets;
> +    group->byte_count += stats->n_bytes;
> +
> +    if (bucket_index >= 0) { /* Credit to a single bucket.  */
> +        struct bucket_counter *bucket = &group->bucket_stats[bucket_index];
> +
> +        if (bucket_index > group->up.n_buckets) {
> +            OVS_NOT_REACHED();
> +        }
> +
> +        bucket->packet_count += stats->n_packets;
> +        bucket->byte_count += stats->n_bytes;
> +    } else { /* Credit to all buckets.  */
> +        int i;
> +
> +        for (i = 0; i < group->up.n_buckets; i++) {
> +            struct bucket_counter *bucket = &group->bucket_stats[i];
> +
> +            bucket->packet_count += stats->n_packets;
> +            bucket->byte_count += stats->n_bytes;
> +        }
> +    }
> +
> +    ovs_mutex_unlock(&group->stats_mutex);
> +}
>
>  /* Sends 'packet' out 'ofport'.
>   * May modify 'packet'.
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index ed0aa90..c10fe04 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -128,6 +128,9 @@ void group_dpif_get_buckets(const struct group_dpif *group,
>                              const struct list **buckets);
>  enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group);
>
> +void group_dpif_credit_stats(struct group_dpif *group, int bucket_index,
> +                             const struct dpif_flow_stats *stats);
> +
>  bool ofproto_has_vlan_splinters(const struct ofproto_dpif *);
>  ofp_port_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
>                                    ofp_port_t realdev_ofp_port,
> --
> 1.7.9.5
>



More information about the dev mailing list