[ovs-dev] [PATCH v4] ofproto: Add reference count for Openflow groups.

[Ryan Wilson]

When adding support for Openflow group and bucket stats, a group entry is added
to the xlate cache. If the group is removed from ofproto configuration, we need
to guarantee the group exists in the case where stats are attributed to the
group by a handler thread and the xlate cache has not yet been cleared by a
revalidator thread.

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
v3: Split group reference count, support for group and bucket stats into 2
patches. Commit Andy Zhou's test patch separately.
v4: Addressed Joe's comments making it more clear why group refcount is needed
for the xlate cache.
---
 ofproto/ofproto-dpif.h     |   16 ++++++++++++++++
 ofproto/ofproto-provider.h |   12 ++++++++++++
 ofproto/ofproto.c          |   32 +++++++++++++++++++++++++++-----
 3 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index e49616e..345117a 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -232,6 +232,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..181dffb 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -488,6 +488,15 @@ struct ofgroup {
     uint32_t group_id;
     enum ofp11_group_type type; /* One of OFPGT_*. */
 
+    /* Number of references.
+     *
+     * This is needed to keep track of a xlate cache reference to the group.
+     *
+     * If the group is removed from ofproto, we need to guarantee the group
+     * exists in the case where stats are attributed to the group and the xlate
+     * cache has not yet  been cleared by a revalidator thread. */
+    struct ovs_refcount ref_count;
+
     long long int created;      /* Creation time. */
     long long int modified;     /* Time of last modification. */
 
@@ -502,6 +511,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.
  *
  *
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 9b26da7..b5a59f8 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2647,6 +2647,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);
 
@@ -5622,6 +5647,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);
@@ -5752,12 +5778,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. */
-- 
1.7.9.5