ofproto: Postpone sending flow removed messages.

The final flow stats are available only after there are no references
to the rule.  Postpone sending the flow removed message until the
final stats are available.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/include/openflow/openflow-common.h b/include/openflow/openflow-common.h
index e4fecce..d32213f 100644 (file)
--- a/include/openflow/openflow-common.h
+++ b/include/openflow/openflow-common.h
@@ -301,6 +301,8 @@ enum ofp_flow_removed_reason {
     OFPRR_GROUP_DELETE,         /* Group was removed. */
     OFPRR_METER_DELETE,         /* Meter was removed. */
     OFPRR_EVICTION,             /* Switch eviction to free resources. */
+
+    OVS_OFPRR_NONE              /* OVS internal_use only, keep last!. */
 };
 
 /* What changed about the physical port */

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 96e65a7..2ac11b1 100644 (file)
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -865,6 +865,7 @@ ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason,
         return "eviction";
     case OFPRR_METER_DELETE:
         return "meter_delete";
+    case OVS_OFPRR_NONE:
     default:
         snprintf(reasonbuf, bufsize, "%d", (int) reason);
         return reasonbuf;

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index f248f59..0d25f68 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -356,6 +356,11 @@ struct rule {
     /* Eviction precedence. */
     uint16_t importance OVS_GUARDED;
 
+    /* Removal reason for sending flow removed message.
+     * Used only if 'flags' has OFPUTIL_FF_SEND_FLOW_REM set and if the
+     * value is not OVS_OFPRR_NONE. */
+    uint8_t removed_reason;
+
     /* Eviction groups (see comment on struct eviction_group for explanation) .
      *
      * 'eviction_group' is this rule's eviction group, or NULL if it is not in

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 2e14e8c..cf5fa34 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -232,7 +232,8 @@ struct ofport_usage {
 };
 
 /* rule. */
-static void ofproto_rule_send_removed(struct rule *, uint8_t reason);
+static void ofproto_rule_send_removed(struct rule *)
+        OVS_EXCLUDED(ofproto_mutex);
 static bool rule_is_readonly(const struct rule *);
 static void ofproto_rule_insert__(struct ofproto *, struct rule *)
     OVS_REQUIRES(ofproto_mutex);
@@ -2758,7 +2759,14 @@ ofproto_rule_destroy__(struct rule *rule)
 
 static void
 rule_destroy_cb(struct rule *rule)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
+    /* Send rule removed if needed. */
+    if (rule->flags & OFPUTIL_FF_SEND_FLOW_REM
+        && rule->removed_reason != OVS_OFPRR_NONE
+        && !rule_is_hidden(rule)) {
+        ofproto_rule_send_removed(rule);
+    }
     rule->ofproto->ofproto_class->rule_destruct(rule);
     ofproto_rule_destroy__(rule);
 }
@@ -4607,6 +4615,7 @@ replace_rule_create(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     rule->idle_timeout = fm->idle_timeout;
     rule->hard_timeout = fm->hard_timeout;
     rule->importance = fm->importance;
+    rule->removed_reason = OVS_OFPRR_NONE;
 
     *CONST_CAST(uint8_t *, &rule->table_id) = table_id;
     rule->flags = fm->flags & OFPUTIL_FF_STATE;
@@ -4753,9 +4762,7 @@ replace_rule_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
         } else {
             /* XXX: This is slight duplication with delete_flows_finish__() */
 
-            /* XXX: This call should done when rule's refcount reaches
-             * zero to get accurate stats in the flow removed message. */
-            ofproto_rule_send_removed(old_rule, OFPRR_EVICTION);
+            old_rule->removed_reason = OFPRR_EVICTION;
 
             ofmonitor_report(ofproto->connmgr, old_rule, NXFME_DELETED,
                              OFPRR_EVICTION,
@@ -4960,7 +4967,10 @@ delete_flows_finish__(struct ofproto *ofproto,
         for (size_t i = 0; i < rules->n; i++) {
             struct rule *rule = rules->rules[i];
 
-            ofproto_rule_send_removed(rule, reason);
+            /* This value will be used to send the flow removed message right
+             * before the rule is actually destroyed. */
+            rule->removed_reason = reason;
+
             ofmonitor_report(ofproto->connmgr, rule, NXFME_DELETED, reason,
                              req ? req->ofconn : NULL,
                              req ? req->request->xid : 0, NULL);
@@ -5071,23 +5081,20 @@ delete_flow_start_strict(struct ofproto *ofproto,
     return error;
 }
 
-/* XXX: This should be sent right when the rule refcount gets to zero! */
+/* This may only be called by rule_destroy_cb()! */
 static void
-ofproto_rule_send_removed(struct rule *rule, uint8_t reason)
-    OVS_REQUIRES(ofproto_mutex)
+ofproto_rule_send_removed(struct rule *rule)
+    OVS_EXCLUDED(ofproto_mutex)
 {
     struct ofputil_flow_removed fr;
     long long int used;
 
-    if (rule_is_hidden(rule) ||
-        !(rule->flags & OFPUTIL_FF_SEND_FLOW_REM)) {
-        return;
-    }
-
     minimatch_expand(&rule->cr.match, &fr.match);
     fr.priority = rule->cr.priority;
+
+    ovs_mutex_lock(&ofproto_mutex);
     fr.cookie = rule->flow_cookie;
-    fr.reason = reason;
+    fr.reason = rule->removed_reason;
     fr.table_id = rule->table_id;
     calc_duration(rule->created, time_msec(),
                   &fr.duration_sec, &fr.duration_nsec);
@@ -5097,8 +5104,8 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t reason)
     ovs_mutex_unlock(&rule->mutex);
     rule->ofproto->ofproto_class->rule_get_stats(rule, &fr.packet_count,
                                                  &fr.byte_count, &used);
-
     connmgr_send_flow_removed(rule->ofproto->connmgr, &fr);
+    ovs_mutex_unlock(&ofproto_mutex);
 }
 
 /* Sends an OpenFlow "flow removed" message with the given 'reason' (either