diff mbox

[ovs-dev,v3,05/13] connmgr: Make connmgr_wants_packet_in_on_miss() lock-free.

Message ID 1473713563-107204-6-git-send-email-jarno@ovn.org
State Accepted
Headers show

Commit Message

Jarno Rajahalme Sept. 12, 2016, 8:52 p.m. UTC
Make connmgr_wants_packet_in_on_miss() use an atomic int instead of a
list traversal taking the 'ofproto_mutex'.  This allows
connmgr_wants_packet_in_on_miss() to be called also when
'ofproto_mutex' is already held, and makes it faster, too.

Remove unused ofproto_dpif_wants_packet_in_on_miss().

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
v3: Fix the totally broken behavior with a help of a per-ofconn boolean.

 ofproto/connmgr.c      | 80 ++++++++++++++++++++++++++++++++++----------------
 ofproto/ofproto-dpif.c | 12 --------
 ofproto/ofproto-dpif.h |  1 -
 3 files changed, 54 insertions(+), 39 deletions(-)

Comments

Ben Pfaff Sept. 13, 2016, 7:38 p.m. UTC | #1
On Mon, Sep 12, 2016 at 01:52:35PM -0700, Jarno Rajahalme wrote:
> Make connmgr_wants_packet_in_on_miss() use an atomic int instead of a
> list traversal taking the 'ofproto_mutex'.  This allows
> connmgr_wants_packet_in_on_miss() to be called also when
> 'ofproto_mutex' is already held, and makes it faster, too.
> 
> Remove unused ofproto_dpif_wants_packet_in_on_miss().
> 
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> ---
> v3: Fix the totally broken behavior with a help of a per-ofconn boolean.

I think that update_want_packet_in_on_miss() can be a little more
straightforward.  How about this?  I have not tested it.

static void
update_want_packet_in_on_miss(struct ofconn *ofconn)
{
    /* We want a packet-in on miss when controller_id is zero and OpenFlow is
     * lower than version 1.3. */
    enum ofputil_protocol p = ofconn->protocol;
    int new_want = (ofconn->controller_id == 0 &&
                    (p == OFPUTIL_P_NONE ||
                     ofputil_protocol_to_ofp_version(p) < OFP13_VERSION));

    /* Update the setting and the count if ncessary. */
    int old_want = ofconn->want_packet_in_on_miss;
    if (old_want != new_want) {
        atomic_int *dst = &ofconn->connmgr->want_packet_in_on_miss;
        int count;
        atomic_read_relaxed(dst, &count);
        atomic_store_relaxed(dst, count - old_want + new_want);

        ofconn->want_packet_in_on_miss = new_want;
    }
}

Acked-by: Ben Pfaff <blp@ovn.org>
Jarno Rajahalme Sept. 13, 2016, 10:09 p.m. UTC | #2
> On Sep 13, 2016, at 12:38 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Mon, Sep 12, 2016 at 01:52:35PM -0700, Jarno Rajahalme wrote:
>> Make connmgr_wants_packet_in_on_miss() use an atomic int instead of a
>> list traversal taking the 'ofproto_mutex'.  This allows
>> connmgr_wants_packet_in_on_miss() to be called also when
>> 'ofproto_mutex' is already held, and makes it faster, too.
>> 
>> Remove unused ofproto_dpif_wants_packet_in_on_miss().
>> 
>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>> ---
>> v3: Fix the totally broken behavior with a help of a per-ofconn boolean.
> 
> I think that update_want_packet_in_on_miss() can be a little more
> straightforward.  How about this?  I have not tested it.
> 
> static void
> update_want_packet_in_on_miss(struct ofconn *ofconn)
> {
>    /* We want a packet-in on miss when controller_id is zero and OpenFlow is
>     * lower than version 1.3. */
>    enum ofputil_protocol p = ofconn->protocol;
>    int new_want = (ofconn->controller_id == 0 &&
>                    (p == OFPUTIL_P_NONE ||
>                     ofputil_protocol_to_ofp_version(p) < OFP13_VERSION));
> 
>    /* Update the setting and the count if ncessary. */
>    int old_want = ofconn->want_packet_in_on_miss;
>    if (old_want != new_want) {
>        atomic_int *dst = &ofconn->connmgr->want_packet_in_on_miss;
>        int count;
>        atomic_read_relaxed(dst, &count);
>        atomic_store_relaxed(dst, count - old_want + new_want);
> 
>        ofconn->want_packet_in_on_miss = new_want;
>    }
> }
> 

Thanks for the review, I adopted this improvement.

  Jarno

> Acked-by: Ben Pfaff <blp@ovn.org>
diff mbox

Patch

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 6014899..11ac08a 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -32,6 +32,7 @@ 
 #include "openvswitch/ofpbuf.h"
 #include "openvswitch/vconn.h"
 #include "openvswitch/vlog.h"
+#include "ovs-atomic.h"
 #include "pinsched.h"
 #include "poll-loop.h"
 #include "rconn.h"
@@ -65,6 +66,7 @@  struct ofconn {
     enum ofconn_type type;      /* Type. */
     enum ofproto_band band;     /* In-band or out-of-band? */
     bool enable_async_msgs;     /* Initially enable async messages? */
+    bool want_packet_in_on_miss;
 
 /* State that should be cleared from one connection to the next. */
 
@@ -219,6 +221,8 @@  struct connmgr {
     struct sockaddr_in *extra_in_band_remotes;
     size_t n_extra_remotes;
     int in_band_queue;
+
+    ATOMIC(int) want_packet_in_on_miss;   /* Sum of ofconns' values. */
 };
 
 static void update_in_band_remotes(struct connmgr *);
@@ -258,9 +262,46 @@  connmgr_create(struct ofproto *ofproto,
     mgr->n_extra_remotes = 0;
     mgr->in_band_queue = -1;
 
+    atomic_init(&mgr->want_packet_in_on_miss, 0);
+
     return mgr;
 }
 
+/* The default "table-miss" behaviour for OpenFlow1.3+ is to drop the
+ * packet rather than to send the packet to the controller.
+ *
+ * This function maintains the count of pre-OpenFlow1.3 with controller_id 0,
+ * as we assume these are the controllers that should receive "table-miss"
+ * notifications. */
+static void
+update_want_packet_in_on_miss(struct ofconn *ofconn)
+{
+    int want_packet_in_on_miss_delta = 0;
+
+    /* Set ofconn->want_packet_in_on_miss when controller_id is zero
+     * and OpenFlow is lower than version 1.3, otherwise clear it. */
+    if (ofconn->controller_id == 0 &&
+        (ofconn->protocol == OFPUTIL_P_NONE ||
+         ofputil_protocol_to_ofp_version(ofconn->protocol) < OFP13_VERSION)) {
+        if (!ofconn->want_packet_in_on_miss) {
+            ofconn->want_packet_in_on_miss = true;
+            want_packet_in_on_miss_delta = 1;
+        }
+    } else {
+        if (ofconn->want_packet_in_on_miss) {
+            ofconn->want_packet_in_on_miss = false;
+            want_packet_in_on_miss_delta = -1;
+        }
+    }
+    if (want_packet_in_on_miss_delta) {
+        int count;
+        atomic_read_relaxed(&ofconn->connmgr->want_packet_in_on_miss,
+                            &count);
+        atomic_store_relaxed(&ofconn->connmgr->want_packet_in_on_miss,
+                             count + want_packet_in_on_miss_delta);
+    }
+}
+
 /* Frees 'mgr' and all of its resources. */
 void
 connmgr_destroy(struct connmgr *mgr)
@@ -1001,6 +1042,7 @@  void
 ofconn_set_protocol(struct ofconn *ofconn, enum ofputil_protocol protocol)
 {
     ofconn->protocol = protocol;
+    update_want_packet_in_on_miss(ofconn);
 }
 
 /* Returns the currently configured packet in format for 'ofconn', one of
@@ -1030,6 +1072,7 @@  void
 ofconn_set_controller_id(struct ofconn *ofconn, uint16_t controller_id)
 {
     ofconn->controller_id = controller_id;
+    update_want_packet_in_on_miss(ofconn);
 }
 
 /* Returns the default miss send length for 'ofconn'. */
@@ -1304,6 +1347,11 @@  ofconn_destroy(struct ofconn *ofconn)
 {
     ofconn_flush(ofconn);
 
+    /* Force clearing of want_packet_in_on_miss to keep the global count
+     * accurate. */
+    ofconn->controller_id = 1;
+    update_want_packet_in_on_miss(ofconn);
+
     if (ofconn->type == OFCONN_PRIMARY) {
         hmap_remove(&ofconn->connmgr->controllers, &ofconn->hmap_node);
     }
@@ -1492,37 +1540,17 @@  ofconn_receives_async_msg(const struct ofconn *ofconn,
     return (masks[type] & (1u << reason)) != 0;
 }
 
-/* The default "table-miss" behaviour for OpenFlow1.3+ is to drop the
- * packet rather than to send the packet to the controller.
- *
- * This function returns true to indicate that a packet_in message
+/* This function returns true to indicate that a packet_in message
  * for a "table-miss" should be sent to at least one controller.
- * That is there is at least one controller with controller_id 0
- * which connected using an OpenFlow version earlier than OpenFlow1.3.
  *
- * False otherwise.
- *
- * This logic assumes that "table-miss" packet_in messages
- * are always sent to controller_id 0. */
+ * False otherwise. */
 bool
-connmgr_wants_packet_in_on_miss(struct connmgr *mgr) OVS_EXCLUDED(ofproto_mutex)
+connmgr_wants_packet_in_on_miss(struct connmgr *mgr)
 {
-    struct ofconn *ofconn;
-
-    ovs_mutex_lock(&ofproto_mutex);
-    LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
-        enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
+    int count;
 
-        if (ofconn->controller_id == 0 &&
-            (protocol == OFPUTIL_P_NONE ||
-             ofputil_protocol_to_ofp_version(protocol) < OFP13_VERSION)) {
-            ovs_mutex_unlock(&ofproto_mutex);
-            return true;
-        }
-    }
-    ovs_mutex_unlock(&ofproto_mutex);
-
-    return false;
+    atomic_read_relaxed(&mgr->want_packet_in_on_miss, &count);
+    return count > 0;
 }
 
 /* Returns a human-readable name for an OpenFlow connection between 'mgr' and
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 289e7d6..d928f01 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -379,18 +379,6 @@  ofproto_dpif_send_async_msg(struct ofproto_dpif *ofproto,
     /* Wakes up main thread for packet-in I/O. */
     seq_change(ofproto->ams_seq);
 }
-
-/* The default "table-miss" behaviour for OpenFlow1.3+ is to drop the
- * packet rather than to send the packet to the controller.
- *
- * This function returns false to indicate that a packet_in message
- * for a "table-miss" should be sent to at least one controller.
- * False otherwise. */
-bool
-ofproto_dpif_wants_packet_in_on_miss(struct ofproto_dpif *ofproto)
-{
-    return connmgr_wants_packet_in_on_miss(ofproto->up.connmgr);
-}
 
 /* Factory functions. */
 
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index a3b1d6b..f912f6e 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -156,7 +156,6 @@  int ofproto_dpif_execute_actions__(struct ofproto_dpif *, const struct flow *,
                                    struct dp_packet *);
 void ofproto_dpif_send_async_msg(struct ofproto_dpif *,
                                  struct ofproto_async_msg *);
-bool ofproto_dpif_wants_packet_in_on_miss(struct ofproto_dpif *);
 int ofproto_dpif_send_packet(const struct ofport_dpif *, bool oam,
                              struct dp_packet *);
 void ofproto_dpif_flow_mod(struct ofproto_dpif *,