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

[Andy Zhou]

On Wed, May 21, 2014 at 5:12 PM, Ryan Wilson <wryan at nicira.com> wrote:
> When adding support for OpenFlow group and bucket stats, a group entry is added
> to the xlate_cache. If the main thread removes the group from an ofproto, we
> need to guarantee that the group remains accessible to users of
> xlate_group_actions and the xlate_cache, as the xlate_cache will not be cleaned
> up until the corresponding datapath flows are revalidated.
>
> Before, modify_group could change the bucket list for a group. With group
> entries in the xlate_cache, this could leave already-freed bucket pointers in
> the cache. Modify_group now recreates the group and replaces the old group in
> ofproto's ofgroup hash map. Thus, any subsequent group lookup will find the new
> group while the old group and buckets will still exist until the xlate_cache
> entries unref the group.
>
> Since ofgroup's properties were only written in-place by modify_group and this
> is no longer done, this eliminates the need for ofgroup's lock.
>
> 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.
> v5: Added Joe's comments in git commit for more accuracy regarding xlate cache,
> edited modify_group in ofproto.c to be compatible with group ref counting
> v6: Fix lock annotations in ofproto-provider.h
> v7: Add more clear git commit message, addressed Andy's comments.
> ---

I think it is ready with the following incremental. Please let me know
your thoughts, I can fold them in
and push.

1) Fixed a clang reported error.
2) Add a lock annotation.
3) Minor restructure of modify_group() to make it easier to read.
Removed one of the labels.

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c79602c..85156ab 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3627,7 +3627,6 @@ group_get_stats(const struct ofgroup *group_,
struct ofputil_group_stats *ogs)
 bool
 group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
                   struct group_dpif **group)
-    OVS_TRY_RDLOCK(true, (*group)->up.rwlock)
 {
     struct ofgroup *ofgroup;
     bool found;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ad69d4c..efcd4d5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5359,6 +5359,7 @@ handle_meter_request(struct ofconn *ofconn,
const struct ofp_header *request,
 bool
 ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
                      struct ofgroup **group)
+    OVS_EXCLUDED(ofproto->groups_rwlock)
 {
     ovs_rwlock_rdlock(&ofproto->groups_rwlock);
     HMAP_FOR_EACH_IN_BUCKET (*group, hmap_node,
@@ -5699,7 +5700,7 @@ add_group(struct ofproto *ofproto, struct
ofputil_group_mod *gm)
 static enum ofperr
 modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
 {
-    struct ofgroup *ofgroup, *new_ofgroup;
+    struct ofgroup *ofgroup, *new_ofgroup, *retiring;
     enum ofperr error;

     error = init_group(ofproto, gm, &new_ofgroup);
@@ -5707,16 +5708,18 @@ modify_group(struct ofproto *ofproto, struct
ofputil_group_mod *gm)
         return error;
     }

+    retiring = new_ofgroup;
+
     if (!ofproto_group_write_lookup(ofproto, gm->group_id, &ofgroup)) {
         error = OFPERR_OFPGMFC_UNKNOWN_GROUP;
-        goto free_out;
+        goto out;
     }

     /* Ofproto's group write lock is held now. */
     if (ofgroup->type != gm->type
         && ofproto->n_groups[gm->type] >= ofproto->ogf.max_groups[gm->type]) {
         error = OFPERR_OFPGMFC_OUT_OF_GROUPS;
-        goto free_out;
+        goto out;
     }

     /* The group creation time does not change during modification. */
@@ -5725,9 +5728,10 @@ modify_group(struct ofproto *ofproto, struct
ofputil_group_mod *gm)

     error = ofproto->ofproto_class->group_modify(new_ofgroup);
     if (error) {
-        goto free_out;
+        goto out;
     }

+    retiring = ofgroup;
     /* Replace ofgroup in ofproto's groups hash map with new_ofgroup. */
     hmap_remove(&ofproto->groups, &ofgroup->hmap_node);
     hmap_insert(&ofproto->groups, &new_ofgroup->hmap_node,
@@ -5736,12 +5740,9 @@ modify_group(struct ofproto *ofproto, struct
ofputil_group_mod *gm)
         ofproto->n_groups[ofgroup->type]--;
         ofproto->n_groups[new_ofgroup->type]++;
     }
-    ofproto_group_unref(ofgroup);
-    goto unlock_out;

- free_out:
-    ofproto_group_unref(new_ofgroup);
- unlock_out:
+out:
+    ofproto_group_unref(retiring);
     ovs_rwlock_unlock(&ofproto->groups_rwlock);
     return error;
 }