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

Ryan Wilson 76511 wryan at vmware.com
Thu May 22 02:20:46 UTC 2014


Yup, modify_group looks much cleaner! And sorry for the lock annotations;
I typically use GCC but its clear I need to use clang as well. You're
welcome to push with your incremental, thanks!

I'll also send out another version of my group and bucket stats patch,
there are some rebase errors with the previous one due how much this
reference count patch changed.

Ryan

On 5/21/14 7:05 PM, "Andy Zhou" <azhou at nicira.com> wrote:

>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;
> }
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbffg%
>3D%3D%0A&m=Cxw0tUFvnczvVYYbSBh9gUkS6yPySwDnT5Rys%2Fkd1G4%3D%0A&s=7ca862f8a
>b265c2b90da83b7ac083488774991dd3d48169533ea8503dfe77484




More information about the dev mailing list