[ovs-dev] [race-fix 3/6] ofproto-dpif-xlate: Fix fin_timeout to make rules expirable.

Ben Pfaff blp at nicira.com
Tue Aug 27 00:10:02 UTC 2013


In an Open vSwitch flow table that has a configured maximum number of
flows, flows that have an idle or hard timeout, or both, are expirable,
and flows with neither are permanent.  The fin_timeout action can change
a flow that has no idle or hard timeout into one that has either one or
both, which should make a permanent flow into an expirable one, but the
implementation was buggy and did not actually make the flow expirable.
This commit fixes the problem.

This commit also moves most of the implementation of fin_timeout from
ofproto-dpif-xlate into ofproto, because this seems to better respect the
layering.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/ofproto-dpif-xlate.c |   24 ++----------------------
 ofproto/ofproto-provider.h   |    3 +++
 ofproto/ofproto.c            |   41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 54fabfe..eeccbc5 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2111,33 +2111,13 @@ xlate_learn_action(struct xlate_ctx *ctx,
     ofproto_dpif_flow_mod(ctx->xbridge->ofproto, fm);
 }
 
-/* Reduces '*timeout' to no more than 'max'.  A value of zero in either case
- * means "infinite". */
-static void
-reduce_timeout(uint16_t max, uint16_t *timeout)
-{
-    if (max && (!*timeout || *timeout > max)) {
-        *timeout = max;
-    }
-}
-
 static void
 xlate_fin_timeout(struct xlate_ctx *ctx,
                   const struct ofpact_fin_timeout *oft)
 {
     if (ctx->xin->tcp_flags & (TCP_FIN | TCP_RST) && ctx->rule) {
-        struct rule_dpif *rule = ctx->rule;
-
-        ovs_mutex_lock(&rule->up.ofproto->expirable_mutex);
-        if (list_is_empty(&rule->up.expirable)) {
-            list_insert(&rule->up.ofproto->expirable, &rule->up.expirable);
-        }
-        ovs_mutex_unlock(&rule->up.ofproto->expirable_mutex);
-
-        ovs_mutex_lock(&rule->up.timeout_mutex);
-        reduce_timeout(oft->fin_idle_timeout, &rule->up.idle_timeout);
-        reduce_timeout(oft->fin_hard_timeout, &rule->up.hard_timeout);
-        ovs_mutex_unlock(&rule->up.timeout_mutex);
+        ofproto_rule_reduce_timeouts(&ctx->rule->up, oft->fin_idle_timeout,
+                                     oft->fin_hard_timeout);
     }
 }
 
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index d8b6a79..72ba3be 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -281,6 +281,9 @@ void ofproto_rule_expire(struct rule *rule, uint8_t reason)
     OVS_RELEASES(rule->evict);
 void ofproto_rule_destroy(struct ofproto *, struct classifier *cls,
                           struct rule *) OVS_REQ_WRLOCK(cls->rwlock);
+void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout,
+                                  uint16_t hard_timeout)
+    OVS_EXCLUDED(rule->ofproto->expirable_mutex, rule->timeout_mutex);
 
 bool ofproto_rule_has_out_port(const struct rule *, ofp_port_t out_port);
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 709fd07..4e3efbe 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -186,6 +186,7 @@ static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep)
     OVS_TRY_WRLOCK(true, (*rulep)->evict);
 static void ofproto_evict(struct ofproto *);
 static uint32_t rule_eviction_priority(struct rule *);
+static void eviction_group_add_rule(struct rule *rule);
 
 /* ofport. */
 static void ofport_destroy__(struct ofport *);
@@ -3785,6 +3786,46 @@ ofproto_rule_expire(struct rule *rule, uint8_t reason)
     ofproto->ofproto_class->rule_destruct(rule);
     ofopgroup_submit(group);
 }
+
+/* Reduces '*timeout' to no more than 'max'.  A value of zero in either case
+ * means "infinite". */
+static void
+reduce_timeout(uint16_t max, uint16_t *timeout)
+{
+    if (max && (!*timeout || *timeout > max)) {
+        *timeout = max;
+    }
+}
+
+/* If 'idle_timeout' is nonzero, and 'rule' has no idle timeout or an idle
+ * timeout greater than 'idle_timeout', lowers 'rule''s idle timeout to
+ * 'idle_timeout' seconds.  Similarly for 'hard_timeout'.
+ *
+ * Suitable for implementing OFPACT_FIN_TIMEOUT. */
+void
+ofproto_rule_reduce_timeouts(struct rule *rule,
+                             uint16_t idle_timeout, uint16_t hard_timeout)
+    OVS_EXCLUDED(rule->ofproto->expirable_mutex, rule->timeout_mutex)
+{
+    if (!idle_timeout && !hard_timeout) {
+        return;
+    }
+
+    ovs_mutex_lock(&rule->ofproto->expirable_mutex);
+    if (list_is_empty(&rule->expirable)) {
+        list_insert(&rule->ofproto->expirable, &rule->expirable);
+    }
+    ovs_mutex_unlock(&rule->ofproto->expirable_mutex);
+
+    ovs_mutex_lock(&rule->timeout_mutex);
+    reduce_timeout(idle_timeout, &rule->idle_timeout);
+    reduce_timeout(hard_timeout, &rule->hard_timeout);
+    ovs_mutex_unlock(&rule->timeout_mutex);
+
+    if (!rule->eviction_group) {
+        eviction_group_add_rule(rule);
+    }
+}
 
 static enum ofperr
 handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
-- 
1.7.10.4




More information about the dev mailing list