[ovs-dev] [PATCH 03/13] ofproto: Take group references only when needed.

Jarno Rajahalme jarno at ovn.org
Fri Jul 15 10:19:09 UTC 2016


Avoid unnecessary references when RCU protection suffices.  This makes
group lookup memory management more like flow lookup memory
management.

Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 22 +++++++++++++---------
 ofproto/ofproto-dpif.c       |  6 ++++--
 ofproto/ofproto-dpif.h       |  2 +-
 ofproto/ofproto-provider.h   |  2 +-
 ofproto/ofproto.c            |  7 ++++---
 5 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 39b4bc9..6b3be6d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1502,13 +1502,9 @@ group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth)
 {
     struct group_dpif *group;
 
-    group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+    group = group_dpif_lookup(ctx->xbridge->ofproto, group_id, false);
     if (group) {
-        struct ofputil_bucket *bucket;
-
-        bucket = group_first_live_bucket(ctx, group, depth);
-        group_dpif_unref(group);
-        return bucket != NULL;
+        return group_first_live_bucket(ctx, group, depth) != NULL;
     }
 
     return false;
@@ -3376,6 +3372,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
     }
 }
 
+/* Consumes the group reference, which is only taken if xcache exists. */
 static void
 xlate_group_stats(struct xlate_ctx *ctx, struct group_dpif *group,
                   struct ofputil_bucket *bucket)
@@ -3387,7 +3384,7 @@ xlate_group_stats(struct xlate_ctx *ctx, struct group_dpif *group,
         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.group = group;
         entry->u.group.bucket = bucket;
     }
 }
@@ -3465,6 +3462,8 @@ xlate_ff_group(struct xlate_ctx *ctx, struct group_dpif *group)
     if (bucket) {
         xlate_group_bucket(ctx, bucket);
         xlate_group_stats(ctx, group, bucket);
+    } else if (ctx->xin->xcache) {
+        group_dpif_unref(group);
     }
 }
 
@@ -3481,6 +3480,8 @@ xlate_default_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
     if (bucket) {
         xlate_group_bucket(ctx, bucket);
         xlate_group_stats(ctx, group, bucket);
+    } else if (ctx->xin->xcache) {
+        group_dpif_unref(group);
     }
 }
 
@@ -3550,6 +3551,8 @@ xlate_hash_fields_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
     if (bucket) {
         xlate_group_bucket(ctx, bucket);
         xlate_group_stats(ctx, group, bucket);
+    } else if (ctx->xin->xcache) {
+        group_dpif_unref(group);
     }
 }
 
@@ -3595,7 +3598,6 @@ xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group)
     default:
         OVS_NOT_REACHED();
     }
-    group_dpif_unref(group);
 
     ctx->in_group = was_in_group;
 }
@@ -3606,7 +3608,9 @@ xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id)
     if (xlate_resubmit_resource_check(ctx)) {
         struct group_dpif *group;
 
-        group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+        /* Take ref only if xcache exists. */
+        group = group_dpif_lookup(ctx->xbridge->ofproto, group_id,
+                                  ctx->xin->xcache);
         if (!group) {
             /* XXX: Should set ctx->error ? */
             return true;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ab26f38..1d3181a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4352,9 +4352,11 @@ group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
  * Make sure to call group_dpif_unref() after no longer needing to maintain
  * a reference to the group. */
 struct group_dpif *
-group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id)
+group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
+                  bool take_ref)
 {
-    struct ofgroup *ofgroup = ofproto_group_lookup(&ofproto->up, group_id);
+    struct ofgroup *ofgroup = ofproto_group_lookup(&ofproto->up, group_id,
+                                                   take_ref);
     return ofgroup ? group_dpif_cast(ofgroup) : NULL;
 }
 
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 18a90b2..4c6f088 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -137,7 +137,7 @@ void group_dpif_credit_stats(struct group_dpif *,
                              struct ofputil_bucket *,
                              const struct dpif_flow_stats *);
 struct group_dpif *group_dpif_lookup(struct ofproto_dpif *ofproto,
-                                     uint32_t group_id);
+                                     uint32_t group_id, bool take_ref);
 const struct ovs_list *group_dpif_get_buckets(const struct group_dpif *group);
 
 enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index e2bc6f3..1437af7 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -525,7 +525,7 @@ struct ofgroup {
 };
 
 struct ofgroup *ofproto_group_lookup(const struct ofproto *ofproto,
-                                     uint32_t group_id);
+                                      uint32_t group_id, bool take_ref);
 
 void ofproto_group_ref(struct ofgroup *);
 bool ofproto_group_try_ref(struct ofgroup *);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index fb4993f..9173376 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6184,17 +6184,18 @@ ofproto_group_lookup__(const struct ofproto *ofproto, uint32_t group_id)
  * Make sure to call ofproto_group_unref() after no longer needing to maintain
  * a reference to the group. */
 struct ofgroup *
-ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id)
+ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
+                     bool take_ref)
 {
     struct ofgroup *group;
 
     group = ofproto_group_lookup__(ofproto, group_id);
-    if (group) {
+    if (group && take_ref) {
         /* Not holding a lock, so it is possible that another thread releases
          * the last reference just before we manage to get one. */
         return ofproto_group_try_ref(group) ? group : NULL;
     }
-    return NULL;
+    return group;
 }
 
 /* Caller should hold 'ofproto->groups_rwlock' if it is important that the
-- 
2.1.4




More information about the dev mailing list