On Wed, May 21, 2014 at 5:12 PM, Ryan Wilson <wr...@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 <wr...@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@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
