[ovs-dev] [PATCH] ofproto: fix interactions between flow monitors and barriers

[YAMAMOTO Takashi]

Following OpenFlow 1.4 semantics, make barriers wait for
flow monitor replies.  This should fix a race in
"ofproto - flow monitoring pause and resume" test.

Signed-off-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
---
 ofproto/connmgr.c | 27 +++++++++++++++++++++++++++
 ofproto/connmgr.h |  1 +
 ofproto/ofproto.c | 16 +++++++++++++++-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index ea79795..aa9b54c 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1118,6 +1118,13 @@ ofconn_pktbuf_retrieve(struct ofconn *ofconn, uint32_t id,
     return pktbuf_retrieve(ofconn->pktbuf, id, bufferp, in_port);
 }
 
+/* Returns true if 'ofconn' has any pending flow monitor updates. */
+bool
+ofconn_has_pending_monitor(const struct ofconn *ofconn)
+{
+    return !list_is_empty(&ofconn->updates) || ofconn->monitor_paused;
+}
+
 /* Returns true if 'ofconn' has any pending opgroups. */
 bool
 ofconn_has_pending_opgroups(const struct ofconn *ofconn)
@@ -2040,6 +2047,20 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule,
     }
 }
 
+static bool
+ofmonitor_has_pending(struct connmgr *mgr)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    struct ofconn *ofconn;
+
+    LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
+        if (ofconn_has_pending_monitor(ofconn)) {
+            return true;
+        }
+    }
+    return false;
+}
+
 void
 ofmonitor_flush(struct connmgr *mgr)
     OVS_REQUIRES(ofproto_mutex)
@@ -2066,6 +2087,9 @@ ofmonitor_flush(struct connmgr *mgr)
             }
         }
     }
+    if (!ofmonitor_has_pending(mgr)) {
+        connmgr_retry(mgr);
+    }
 }
 
 static void
@@ -2091,6 +2115,9 @@ ofmonitor_resume(struct ofconn *ofconn)
     ofconn_send_replies(ofconn, &msgs);
 
     ofconn->monitor_paused = 0;
+    if (!ofmonitor_has_pending(ofconn->connmgr)) {
+        connmgr_retry(ofconn->connmgr);
+    }
 }
 
 static bool
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 170d872..40b3ff6 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -142,6 +142,7 @@ enum ofperr ofconn_pktbuf_retrieve(struct ofconn *, uint32_t id,
                                    struct ofpbuf **bufferp, ofp_port_t *in_port);
 
 bool ofconn_has_pending_opgroups(const struct ofconn *);
+bool ofconn_has_pending_monitor(const struct ofconn *);
 void ofconn_add_opgroup(struct ofconn *, struct list *);
 void ofconn_remove_opgroup(struct ofconn *, struct list *,
                            const struct ofp_header *request, int error);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 676a6cb..1b59a28 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4678,7 +4678,21 @@ handle_barrier_request(struct ofconn *ofconn, const struct ofp_header *oh)
 {
     struct ofpbuf *buf;
 
-    if (ofconn_has_pending_opgroups(ofconn)) {
+    /*
+     * OpenFlow 1.4.0 p.110:
+     *    A OFPMP_FLOW_MONITOR multipart reply can not cross a barrier
+     *    handshake.  The switch must always send the OFPMP_FLOW_MONITOR
+     *    multipart reply for a given flow table change before the reply
+     *    to a OFPT_BARRIER_REQUEST request that follows the request
+     *    responsible for the flow table change.
+     *
+     * The spec seems unclear what to do for multiple connections, though.
+     * We serialize only flow monitor replies on our connection.
+     * We don't care which connection has made flow modifications triggering
+     * the flow monitor replies.
+     */
+    if (ofconn_has_pending_opgroups(ofconn) ||
+        ofconn_has_pending_monitor(ofconn)) {
         return OFPROTO_POSTPONE;
     }
 
-- 
1.8.3.1