Forgot to add marco.can...@acm.org to CC
On Wed, Apr 9, 2014 at 4:43 PM, Andy Zhou <az...@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.can...@acm.org>
> Signed-off-by: Andy Zhou <az...@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 l...@vmware.com
> Logan Rosen logatron...@gmail.com
> Luca Falavigna dktrkr...@debian.org
> Luiz Henrique Ozaki luiz.oz...@gmail.com
> +Marco Canini marco.can...@acm.org
> Marco d'Itri m...@linux.it
> Maxime Brun m.b...@alphalink.fr
> Michael A. Collins mike.a.coll...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
