[ovs-dev] [PATCHv2] ofproto: Add support for Openflow group and bucket stats.
Ryan Wilson
wryan at nicira.com
Tue May 6 20:05:02 UTC 2014
This patch also adds a ref/unref structure for ofgroup, so handler and
revalidator threads can update group / bucket stats via the xlate cache.
Signed-off-by: Ryan Wilson <wryan at nicira.com>
---
v2: Fixed bug with group stats all buckets, cleaned up ofgroup unref code,
added Andy Zhou's test for more complete test coverage
---
lib/ofp-util.h | 12 ++++----
ofproto/ofproto-dpif-xlate.c | 51 ++++++++++++++++++++++++-------
ofproto/ofproto-dpif.c | 68 ++++++++++++++++++++++++++----------------
ofproto/ofproto-dpif.h | 19 ++++++++++++
ofproto/ofproto-provider.h | 11 ++++++-
ofproto/ofproto.c | 34 +++++++++++++++++----
tests/ofproto-dpif.at | 44 +++++++++++++++++++++++++++
7 files changed, 192 insertions(+), 47 deletions(-)
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 782e512..77967a5 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -1042,6 +1042,11 @@ int ofputil_decode_queue_stats(struct ofputil_queue_stats *qs, struct ofpbuf *ms
void ofputil_append_queue_stat(struct list *replies,
const struct ofputil_queue_stats *oqs);
+struct bucket_counter {
+ uint64_t packet_count; /* Number of packets processed by bucket. */
+ uint64_t byte_count; /* Number of bytes processed by bucket. */
+};
+
/* Bucket for use in groups. */
struct ofputil_bucket {
struct list list_node;
@@ -1054,6 +1059,8 @@ struct ofputil_bucket {
* failover groups. */
struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
size_t ofpacts_len; /* Length of ofpacts, in bytes. */
+
+ struct bucket_counter stats;
};
/* Protocol-independent group_mod. */
@@ -1064,11 +1071,6 @@ struct ofputil_group_mod {
struct list buckets; /* Contains "struct ofputil_bucket"s. */
};
-struct bucket_counter {
- uint64_t packet_count; /* Number of packets processed by bucket. */
- uint64_t byte_count; /* Number of bytes processed by bucket. */
-};
-
/* Group stats reply, independent of protocol. */
struct ofputil_group_stats {
uint32_t group_id; /* Group identifier. */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index da538b7..bfbd0ca 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -235,6 +235,7 @@ enum xc_type {
XC_LEARN,
XC_NORMAL,
XC_FIN_TIMEOUT,
+ XC_GROUP,
};
/* xlate_cache entries hold enough information to perform the side effects of
@@ -279,6 +280,10 @@ struct xc_entry {
uint16_t idle;
uint16_t hard;
} fin;
+ struct {
+ struct group_dpif *group;
+ struct ofputil_bucket *bucket;
+ } group;
} u;
};
@@ -837,7 +842,7 @@ odp_port_is_alive(const struct xlate_ctx *ctx, ofp_port_t ofp_port)
return true;
}
-static const struct ofputil_bucket *
+static struct ofputil_bucket *
group_first_live_bucket(const struct xlate_ctx *, const struct group_dpif *,
int depth);
@@ -862,7 +867,7 @@ group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth)
static bool
bucket_is_alive(const struct xlate_ctx *ctx,
- const struct ofputil_bucket *bucket, int depth)
+ struct ofputil_bucket *bucket, int depth)
{
if (depth >= MAX_LIVENESS_RECURSION) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
@@ -879,7 +884,7 @@ bucket_is_alive(const struct xlate_ctx *ctx,
group_is_alive(ctx, bucket->watch_group, depth + 1));
}
-static const struct ofputil_bucket *
+static struct ofputil_bucket *
group_first_live_bucket(const struct xlate_ctx *ctx,
const struct group_dpif *group, int depth)
{
@@ -896,16 +901,16 @@ group_first_live_bucket(const struct xlate_ctx *ctx,
return NULL;
}
-static const struct ofputil_bucket *
+static struct ofputil_bucket *
group_best_live_bucket(const struct xlate_ctx *ctx,
const struct group_dpif *group,
uint32_t basis)
{
- const struct ofputil_bucket *best_bucket = NULL;
+ struct ofputil_bucket *best_bucket = NULL;
uint32_t best_score = 0;
int i = 0;
- const struct ofputil_bucket *bucket;
+ struct ofputil_bucket *bucket;
const struct list *buckets;
group_dpif_get_buckets(group, &buckets);
@@ -2112,7 +2117,23 @@ match:
}
static void
-xlate_group_bucket(struct xlate_ctx *ctx, const struct ofputil_bucket *bucket)
+xlate_group_stats(struct xlate_ctx *ctx, struct group_dpif *group,
+ struct ofputil_bucket *bucket)
+{
+ if (ctx->xin->resubmit_stats) {
+ group_dpif_credit_stats(group, bucket, ctx->xin->resubmit_stats);
+ }
+ if (ctx->xin->xcache) {
+ struct xc_entry *entry;
+
+ entry = xlate_cache_add_entry(ctx->xin->xcache, XC_GROUP);
+ entry->u.group.group = group_dpif_ref(group);
+ entry->u.group.bucket = bucket;
+ }
+}
+
+static void
+xlate_group_bucket(struct xlate_ctx *ctx, struct ofputil_bucket *bucket)
{
uint64_t action_list_stub[1024 / 8];
struct ofpbuf action_list, action_set;
@@ -2132,7 +2153,7 @@ xlate_group_bucket(struct xlate_ctx *ctx, const struct ofputil_bucket *bucket)
static void
xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group)
{
- const struct ofputil_bucket *bucket;
+ struct ofputil_bucket *bucket;
const struct list *buckets;
struct flow old_flow = ctx->xin->flow;
@@ -2148,16 +2169,18 @@ xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group)
* just before applying the all or indirect group. */
ctx->xin->flow = old_flow;
}
+ xlate_group_stats(ctx, group, NULL);
}
static void
xlate_ff_group(struct xlate_ctx *ctx, struct group_dpif *group)
{
- const struct ofputil_bucket *bucket;
+ struct ofputil_bucket *bucket;
bucket = group_first_live_bucket(ctx, group, 0);
if (bucket) {
xlate_group_bucket(ctx, bucket);
+ xlate_group_stats(ctx, group, bucket);
}
}
@@ -2165,7 +2188,7 @@ static void
xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
{
struct flow_wildcards *wc = &ctx->xout->wc;
- const struct ofputil_bucket *bucket;
+ struct ofputil_bucket *bucket;
uint32_t basis;
basis = hash_mac(ctx->xin->flow.dl_dst, 0, 0);
@@ -2173,6 +2196,7 @@ xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
if (bucket) {
memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
xlate_group_bucket(ctx, bucket);
+ xlate_group_stats(ctx, group, bucket);
}
}
@@ -3598,6 +3622,10 @@ xlate_push_stats(struct xlate_cache *xcache, bool may_learn,
xlate_fin_timeout__(entry->u.fin.rule, stats->tcp_flags,
entry->u.fin.idle, entry->u.fin.hard);
break;
+ case XC_GROUP:
+ group_dpif_credit_stats(entry->u.group.group, entry->u.group.bucket,
+ stats);
+ break;
default:
OVS_NOT_REACHED();
}
@@ -3666,6 +3694,9 @@ xlate_cache_clear(struct xlate_cache *xcache)
/* 'u.fin.rule' is always already held as a XC_RULE, which
* has already released it's reference above. */
break;
+ case XC_GROUP:
+ group_dpif_unref(entry->u.group.group);
+ break;
default:
OVS_NOT_REACHED();
}
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e50b4fe..6a3b6c2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -107,7 +107,6 @@ struct group_dpif {
struct ovs_mutex stats_mutex;
uint64_t packet_count OVS_GUARDED; /* Number of packets received. */
uint64_t byte_count OVS_GUARDED; /* Number of bytes received. */
- struct bucket_counter *bucket_stats OVS_GUARDED; /* Bucket statistics. */
};
struct ofbundle {
@@ -3541,17 +3540,42 @@ static void
group_construct_stats(struct group_dpif *group)
OVS_REQUIRES(group->stats_mutex)
{
+ struct ofputil_bucket *bucket;
+ const struct list *buckets;
+
group->packet_count = 0;
group->byte_count = 0;
- if (!group->bucket_stats) {
- group->bucket_stats = xcalloc(group->up.n_buckets,
- sizeof *group->bucket_stats);
- } else {
- memset(group->bucket_stats, 0, group->up.n_buckets *
- sizeof *group->bucket_stats);
+
+ group_dpif_get_buckets(group, &buckets);
+ LIST_FOR_EACH (bucket, list_node, buckets) {
+ bucket->stats.packet_count = 0;
+ bucket->stats.byte_count = 0;
}
}
+void
+group_dpif_credit_stats(struct group_dpif *group,
+ struct ofputil_bucket *bucket,
+ const struct dpif_flow_stats *stats)
+{
+ ovs_mutex_lock(&group->stats_mutex);
+ group->packet_count += stats->n_packets;
+ group->byte_count += stats->n_bytes;
+ if (bucket) {
+ bucket->stats.packet_count += stats->n_packets;
+ bucket->stats.byte_count += stats->n_bytes;
+ } else { /* Credit to all buckets */
+ const struct list *buckets;
+
+ group_dpif_get_buckets(group, &buckets);
+ LIST_FOR_EACH (bucket, list_node, buckets) {
+ bucket->stats.packet_count += stats->n_packets;
+ bucket->stats.byte_count += stats->n_bytes;
+ }
+ }
+ ovs_mutex_unlock(&group->stats_mutex);
+}
+
static enum ofperr
group_construct(struct ofgroup *group_)
{
@@ -3578,34 +3602,19 @@ group_construct(struct ofgroup *group_)
}
static void
-group_destruct__(struct group_dpif *group)
- OVS_REQUIRES(group->stats_mutex)
-{
- free(group->bucket_stats);
- group->bucket_stats = NULL;
-}
-
-static void
group_destruct(struct ofgroup *group_)
{
struct group_dpif *group = group_dpif_cast(group_);
- ovs_mutex_lock(&group->stats_mutex);
- group_destruct__(group);
- ovs_mutex_unlock(&group->stats_mutex);
ovs_mutex_destroy(&group->stats_mutex);
}
static enum ofperr
-group_modify(struct ofgroup *group_, struct ofgroup *victim_)
+group_modify(struct ofgroup *group_)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(group_->ofproto);
struct group_dpif *group = group_dpif_cast(group_);
- struct group_dpif *victim = group_dpif_cast(victim_);
ovs_mutex_lock(&group->stats_mutex);
- if (victim->up.n_buckets < group->up.n_buckets) {
- group_destruct__(group);
- }
group_construct_stats(group);
ovs_mutex_unlock(&group->stats_mutex);
@@ -3618,12 +3627,21 @@ static enum ofperr
group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
{
struct group_dpif *group = group_dpif_cast(group_);
+ struct ofputil_bucket *bucket;
+ const struct list *buckets;
+ struct bucket_counter *bucket_stats;
ovs_mutex_lock(&group->stats_mutex);
ogs->packet_count = group->packet_count;
ogs->byte_count = group->byte_count;
- memcpy(ogs->bucket_stats, group->bucket_stats,
- group->up.n_buckets * sizeof *group->bucket_stats);
+
+ group_dpif_get_buckets(group, &buckets);
+ bucket_stats = ogs->bucket_stats;
+ LIST_FOR_EACH (bucket, list_node, buckets) {
+ bucket_stats->packet_count = bucket->stats.packet_count;
+ bucket_stats->byte_count = bucket->stats.byte_count;
+ bucket_stats++;
+ }
ovs_mutex_unlock(&group->stats_mutex);
return 0;
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index d4ad624..f574578 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -125,6 +125,9 @@ void choose_miss_rule(enum ofputil_port_config,
struct rule_dpif *no_packet_in_rule,
struct rule_dpif **rule, bool take_ref);
+void group_dpif_credit_stats(struct group_dpif *,
+ struct ofputil_bucket *,
+ const struct dpif_flow_stats *);
bool group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
struct group_dpif **group);
@@ -232,6 +235,22 @@ BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
/* struct rule_dpif has struct rule as it's first member. */
#define RULE_CAST(RULE) ((struct rule *)RULE)
+#define GROUP_CAST(GROUP) ((struct ofgroup *) GROUP)
+
+static inline struct group_dpif* group_dpif_ref(struct group_dpif *group)
+{
+ if (group) {
+ ofproto_group_ref(GROUP_CAST(group));
+ }
+ return group;
+}
+
+static inline void group_dpif_unref(struct group_dpif *group)
+{
+ if (group) {
+ ofproto_group_unref(GROUP_CAST(group));
+ }
+}
static inline void rule_dpif_ref(struct rule_dpif *rule)
{
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 141adec..f01a1b7 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -488,6 +488,12 @@ struct ofgroup {
uint32_t group_id;
enum ofp11_group_type type; /* One of OFPGT_*. */
+ /* Number of references.
+ * The classifier owns one reference.
+ * Any thread trying to keep a rule from being freed should hold its own
+ * reference, typically the xlate cache. */
+ struct ovs_refcount ref_count;
+
long long int created; /* Creation time. */
long long int modified; /* Time of last modification. */
@@ -502,6 +508,9 @@ bool ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
void ofproto_group_release(struct ofgroup *group)
OVS_RELEASES(group->rwlock);
+void ofproto_group_ref(struct ofgroup *);
+void ofproto_group_unref(struct ofgroup *);
+
/* ofproto class structure, to be defined by each ofproto implementation.
*
*
@@ -1684,7 +1693,7 @@ struct ofproto_class {
void (*group_destruct)(struct ofgroup *);
void (*group_dealloc)(struct ofgroup *);
- enum ofperr (*group_modify)(struct ofgroup *, struct ofgroup *victim);
+ enum ofperr (*group_modify)(struct ofgroup *);
enum ofperr (*group_get_stats)(const struct ofgroup *,
struct ofputil_group_stats *);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index d92bafd..b0aa707 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2642,6 +2642,31 @@ ofproto_rule_unref(struct rule *rule)
}
}
+static void
+group_destroy_cb(struct ofgroup *group)
+{
+ group->ofproto->ofproto_class->group_destruct(group);
+ ofputil_bucket_list_destroy(&group->buckets);
+ ovs_rwlock_destroy(&group->rwlock);
+ group->ofproto->ofproto_class->group_dealloc(group);
+}
+
+void
+ofproto_group_ref(struct ofgroup *group)
+{
+ if (group) {
+ ovs_refcount_ref(&group->ref_count);
+ }
+}
+
+void
+ofproto_group_unref(struct ofgroup *group)
+{
+ if (group && ovs_refcount_unref(&group->ref_count) == 1) {
+ ovsrcu_postpone(group_destroy_cb, group);
+ }
+}
+
static uint32_t get_provider_meter_id(const struct ofproto *,
uint32_t of_meter_id);
@@ -5608,6 +5633,7 @@ add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
ofgroup->group_id = gm->group_id;
ofgroup->type = gm->type;
ofgroup->created = ofgroup->modified = time_msec();
+ ovs_refcount_init(&ofgroup->ref_count);
list_move(&ofgroup->buckets, &gm->buckets);
ofgroup->n_buckets = list_size(&ofgroup->buckets);
@@ -5697,7 +5723,7 @@ modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
list_move(&ofgroup->buckets, &gm->buckets);
ofgroup->n_buckets = list_size(&ofgroup->buckets);
- error = ofproto->ofproto_class->group_modify(ofgroup, victim);
+ error = ofproto->ofproto_class->group_modify(ofgroup);
if (!error) {
ofputil_bucket_list_destroy(&victim->buckets);
ofproto->n_groups[victim->type]--;
@@ -5738,12 +5764,8 @@ delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup)
/* No-one can find this group any more. */
ofproto->n_groups[ofgroup->type]--;
ovs_rwlock_unlock(&ofproto->groups_rwlock);
-
- ofproto->ofproto_class->group_destruct(ofgroup);
- ofputil_bucket_list_destroy(&ofgroup->buckets);
ovs_rwlock_unlock(&ofgroup->rwlock);
- ovs_rwlock_destroy(&ofgroup->rwlock);
- ofproto->ofproto_class->group_dealloc(ofgroup);
+ ofproto_group_unref(ofgroup);
}
/* Implements OFPGC_DELETE. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index c46e997..f7d2684 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -381,6 +381,50 @@ AT_CHECK([tail -1 stdout], [0],
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - group stats single bucket])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [10], [11])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=select,bucket=output:10,weight=2000,bucket=output:11,weight=0'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=write_actions(group:1234)'])
+(
+for i in `seq 0 2`;
+ do
+ pkt="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)"
+ AT_CHECK([ovs-appctl netdev-dummy/receive p1 $pkt])
+ done
+)
+ovs-appctl time/warp 100
+sleep 1 # wait for forwarders process packets
+AT_CHECK([ovs-ofctl -O OpenFlow12 -vwarn dump-group-stats br0], [0], [stdout])
+AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl
+ group_id=1234,ref_count=0,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=0,byte_count=0
+OFPST_GROUP reply (OF1.2):
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - group stats all buckets])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [10], [11])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=all,bucket=output:10,bucket=output:11'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=write_actions(group:1234)'])
+(
+for i in `seq 0 2`;
+ do
+ pkt="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)"
+ AT_CHECK([ovs-appctl netdev-dummy/receive p1 $pkt])
+ done
+)
+ovs-appctl time/warp 100
+sleep 1 # wait for forwarders process packets
+AT_CHECK([ovs-ofctl -O OpenFlow12 -vwarn dump-group-stats br0], [0], [stdout])
+AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl
+ group_id=1234,ref_count=0,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=3,byte_count=180
+OFPST_GROUP reply (OF1.2):
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - registers])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [20], [21], [22], [33], [90])
--
1.7.9.5
More information about the dev
mailing list