[ovs-dev] [PATCH] ofproto-dpif: Move special upcall handling into ofproto-dpif-upcall.

Ethan Jackson ethan at nicira.com
Tue Sep 24 22:12:37 UTC 2013


Both the IPFIX and SFLOW modules are thread safe, so there's no
particular reason to pass them up to the main thread.  Eliminating
this step significantly simplifies the code.

Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
 ofproto/ofproto-dpif-upcall.c |  250 +++++++++++++++++++++++++++--------------
 ofproto/ofproto-dpif-upcall.h |   32 ------
 ofproto/ofproto-dpif-xlate.c  |   38 +++++++
 ofproto/ofproto-dpif-xlate.h  |    5 +
 ofproto/ofproto-dpif.c        |   95 ----------------
 5 files changed, 206 insertions(+), 214 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index d75c61b..bfd01b5 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -29,6 +29,8 @@
 #include "list.h"
 #include "netlink.h"
 #include "ofpbuf.h"
+#include "ofproto-dpif-ipfix.h"
+#include "ofproto-dpif-sflow.h"
 #include "ofproto-dpif.h"
 #include "packets.h"
 #include "poll-loop.h"
@@ -83,7 +85,6 @@ struct udpif {
 
     /* Queues to pass up to ofproto-dpif. */
     struct guarded_list drop_keys; /* "struct drop key"s. */
-    struct guarded_list upcalls;   /* "struct upcall"s. */
     struct guarded_list fmbs;      /* "struct flow_miss_batch"es. */
 
     /* Number of times udpif_revalidate() has been called. */
@@ -94,6 +95,29 @@ struct udpif {
     struct latch exit_latch; /* Tells child threads to exit. */
 };
 
+enum upcall_type {
+    /* Handled internally by udpif code.  Not returned by upcall_next().*/
+    BAD_UPCALL,                 /* Some kind of bug somewhere. */
+    MISS_UPCALL,                /* A flow miss.  */
+
+    /* Require main thread's involvement.  May be returned by upcall_next(). */
+    SFLOW_UPCALL,               /* sFlow sample. */
+    FLOW_SAMPLE_UPCALL,         /* Per-flow sampling. */
+    IPFIX_UPCALL                /* Per-bridge sampling. */
+};
+
+struct upcall {
+    struct list list_node;          /* For queuing upcalls. */
+    struct flow_miss *flow_miss;    /* This upcall's flow_miss. */
+
+    /* Raw upcall plus data for keeping track of the memory backing it. */
+    struct dpif_upcall dpif_upcall; /* As returned by dpif_recv() */
+    struct ofpbuf upcall_buf;       /* Owns some data in 'dpif_upcall'. */
+    uint64_t upcall_stub[512 / 8];  /* Buffer to reduce need for malloc(). */
+};
+
+static void upcall_destroy(struct upcall *);
+
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
 static void recv_upcalls(struct udpif *);
@@ -113,7 +137,6 @@ udpif_create(struct dpif_backer *backer, struct dpif *dpif)
     udpif->wait_seq = seq_create();
     latch_init(&udpif->exit_latch);
     guarded_list_init(&udpif->drop_keys);
-    guarded_list_init(&udpif->upcalls);
     guarded_list_init(&udpif->fmbs);
     atomic_init(&udpif->reval_seq, 0);
 
@@ -125,7 +148,6 @@ udpif_destroy(struct udpif *udpif)
 {
     struct flow_miss_batch *fmb;
     struct drop_key *drop_key;
-    struct upcall *upcall;
 
     udpif_recv_set(udpif, 0, false);
 
@@ -133,16 +155,11 @@ udpif_destroy(struct udpif *udpif)
         drop_key_destroy(drop_key);
     }
 
-    while ((upcall = upcall_next(udpif))) {
-        upcall_destroy(upcall);
-    }
-
     while ((fmb = flow_miss_batch_next(udpif))) {
         flow_miss_batch_destroy(fmb);
     }
 
     guarded_list_destroy(&udpif->drop_keys);
-    guarded_list_destroy(&udpif->upcalls);
     guarded_list_destroy(&udpif->fmbs);
     latch_destroy(&udpif->exit_latch);
     seq_destroy(udpif->wait_seq);
@@ -221,7 +238,6 @@ udpif_wait(struct udpif *udpif)
 {
     uint64_t seq = seq_read(udpif->wait_seq);
     if (!guarded_list_is_empty(&udpif->drop_keys) ||
-        !guarded_list_is_empty(&udpif->upcalls) ||
         !guarded_list_is_empty(&udpif->fmbs)) {
         poll_immediate_wake();
     } else {
@@ -254,18 +270,8 @@ udpif_revalidate(struct udpif *udpif)
     udpif_drop_key_clear(udpif);
 }
 
-/* Retreives the next upcall which ofproto-dpif is responsible for handling.
- * The caller is responsible for destroying the returned upcall with
- * upcall_destroy(). */
-struct upcall *
-upcall_next(struct udpif *udpif)
-{
-    struct list *next = guarded_list_pop_front(&udpif->upcalls);
-    return next ? CONTAINER_OF(next, struct upcall, list_node) : NULL;
-}
-
 /* Destroys and deallocates 'upcall'. */
-void
+static void
 upcall_destroy(struct upcall *upcall)
 {
     if (upcall) {
@@ -475,11 +481,129 @@ classify_upcall(const struct upcall *upcall)
 }
 
 static void
+handle_sflow_upcall(struct udpif *udpif, struct upcall *upcall)
+{
+    struct dpif_upcall *dupcall = &upcall->dpif_upcall;
+    union user_action_cookie cookie;
+    struct ofproto_dpif *ofproto;
+    struct dpif_sflow *sflow;
+    odp_port_t odp_in_port;
+    struct flow flow;
+
+    if (xlate_receive(udpif->backer, dupcall->packet, dupcall->key,
+                      dupcall->key_len, &flow, NULL, &ofproto, &odp_in_port)) {
+        return;
+    }
+
+    sflow = xlate_get_sflow(ofproto);
+    if (!sflow) {
+        return;
+    }
+
+    memset(&cookie, 0, sizeof cookie);
+    memcpy(&cookie, nl_attr_get(dupcall->userdata), sizeof cookie.sflow);
+    dpif_sflow_received(sflow, dupcall->packet, &flow, odp_in_port, &cookie);
+    dpif_sflow_unref(sflow);
+}
+
+static void
+handle_ipfix_upcall(struct udpif *udpif, struct upcall *upcall,
+                    bool flow_sample)
+{
+    struct dpif_upcall *dupcall = &upcall->dpif_upcall;
+    struct ofproto_dpif *ofproto;
+    struct dpif_ipfix *ipfix;
+    struct flow flow;
+
+    if (xlate_receive(udpif->backer, dupcall->packet, dupcall->key,
+                      dupcall->key_len, &flow, NULL, &ofproto, NULL)) {
+        return;
+    }
+
+    ipfix = xlate_get_ipfix(ofproto);
+    if (!ipfix) {
+        return;
+    }
+
+    if (flow_sample) {
+        union user_action_cookie cookie;
+
+        memset(&cookie, 0, sizeof cookie);
+        memcpy(&cookie, nl_attr_get(dupcall->userdata),
+               sizeof cookie.flow_sample);
+
+        /* The flow reflects exactly the contents of the packet.  Sample
+         * the packet using it. */
+        dpif_ipfix_flow_sample(ipfix, dupcall->packet, &flow,
+                               cookie.flow_sample.collector_set_id,
+                               cookie.flow_sample.probability,
+                               cookie.flow_sample.obs_domain_id,
+                               cookie.flow_sample.obs_point_id);
+    } else {
+        /* The flow reflects exactly the contents of the packet.  Sample
+         * the packet using it. */
+        dpif_ipfix_bridge_sample(ipfix, dupcall->packet, &flow);
+    }
+    dpif_ipfix_unref(ipfix);
+}
+
+static void
+dispatch_miss_upcall(struct udpif *udpif, struct upcall *upcall)
+{
+    struct dpif_upcall *dupcall = &upcall->dpif_upcall;
+    uint32_t hash = udpif->secret;
+    struct handler *handler;
+    size_t n_bytes, left;
+    struct nlattr *nla;
+
+    n_bytes = 0;
+    NL_ATTR_FOR_EACH (nla, left, dupcall->key, dupcall->key_len) {
+        enum ovs_key_attr type = nl_attr_type(nla);
+        if (type == OVS_KEY_ATTR_IN_PORT
+            || type == OVS_KEY_ATTR_TCP
+            || type == OVS_KEY_ATTR_UDP) {
+            if (nl_attr_get_size(nla) == 4) {
+                ovs_be32 attr = nl_attr_get_be32(nla);
+                hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
+                n_bytes += 4;
+            } else {
+                VLOG_WARN("Netlink attribute with incorrect size.");
+            }
+        }
+    }
+    hash =  mhash_finish(hash, n_bytes);
+
+    handler = &udpif->handlers[hash % udpif->n_handlers];
+
+    ovs_mutex_lock(&handler->mutex);
+    if (handler->n_upcalls < MAX_QUEUE_LENGTH) {
+        list_push_back(&handler->upcalls, &upcall->list_node);
+        handler->n_new_upcalls = ++handler->n_upcalls;
+
+        if (handler->n_new_upcalls >= FLOW_MISS_MAX_BATCH) {
+            xpthread_cond_signal(&handler->wake_cond);
+        }
+        ovs_mutex_unlock(&handler->mutex);
+        if (!VLOG_DROP_DBG(&rl)) {
+            struct ds ds = DS_EMPTY_INITIALIZER;
+
+            odp_flow_key_format(upcall->dpif_upcall.key,
+                                upcall->dpif_upcall.key_len,
+                                &ds);
+            VLOG_DBG("dispatcher: miss enqueue (%s)", ds_cstr(&ds));
+            ds_destroy(&ds);
+        }
+    } else {
+        ovs_mutex_unlock(&handler->mutex);
+        COVERAGE_INC(miss_queue_overflow);
+        upcall_destroy(upcall);
+    }
+}
+
+static void
 recv_upcalls(struct udpif *udpif)
 {
-    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(60, 60);
     size_t n_udpif_new_upcalls = 0;
-    struct handler *handler;
     int n;
 
     for (;;) {
@@ -496,75 +620,27 @@ recv_upcalls(struct udpif *udpif)
             break;
         }
 
-        upcall->type = classify_upcall(upcall);
-        if (upcall->type == BAD_UPCALL) {
+        switch (classify_upcall(upcall)) {
+        case MISS_UPCALL:
+            dispatch_miss_upcall(udpif, upcall);
+            break;
+        case SFLOW_UPCALL:
+            handle_sflow_upcall(udpif, upcall);
+            break;
+        case IPFIX_UPCALL:
+            handle_ipfix_upcall(udpif, upcall, false);
+            break;
+        case FLOW_SAMPLE_UPCALL:
+            handle_ipfix_upcall(udpif, upcall, true);
+            break;
+        case BAD_UPCALL:
             upcall_destroy(upcall);
-        } else if (upcall->type == MISS_UPCALL) {
-            struct dpif_upcall *dupcall = &upcall->dpif_upcall;
-            uint32_t hash = udpif->secret;
-            struct nlattr *nla;
-            size_t n_bytes, left;
-
-            n_bytes = 0;
-            NL_ATTR_FOR_EACH (nla, left, dupcall->key, dupcall->key_len) {
-                enum ovs_key_attr type = nl_attr_type(nla);
-                if (type == OVS_KEY_ATTR_IN_PORT
-                    || type == OVS_KEY_ATTR_TCP
-                    || type == OVS_KEY_ATTR_UDP) {
-                    if (nl_attr_get_size(nla) == 4) {
-                        ovs_be32 attr = nl_attr_get_be32(nla);
-                        hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
-                        n_bytes += 4;
-                    } else {
-                        VLOG_WARN("Netlink attribute with incorrect size.");
-                    }
-                }
-            }
-            hash =  mhash_finish(hash, n_bytes);
-
-            handler = &udpif->handlers[hash % udpif->n_handlers];
-
-            ovs_mutex_lock(&handler->mutex);
-            if (handler->n_upcalls < MAX_QUEUE_LENGTH) {
-                list_push_back(&handler->upcalls, &upcall->list_node);
-                handler->n_new_upcalls = ++handler->n_upcalls;
-
-                if (handler->n_new_upcalls >= FLOW_MISS_MAX_BATCH) {
-                    xpthread_cond_signal(&handler->wake_cond);
-                }
-                ovs_mutex_unlock(&handler->mutex);
-                if (!VLOG_DROP_DBG(&rl)) {
-                    struct ds ds = DS_EMPTY_INITIALIZER;
-
-                    odp_flow_key_format(upcall->dpif_upcall.key,
-                                        upcall->dpif_upcall.key_len,
-                                        &ds);
-                    VLOG_DBG("dispatcher: miss enqueue (%s)", ds_cstr(&ds));
-                    ds_destroy(&ds);
-                }
-            } else {
-                ovs_mutex_unlock(&handler->mutex);
-                COVERAGE_INC(miss_queue_overflow);
-                upcall_destroy(upcall);
-            }
-        } else {
-            size_t len;
-
-            len = guarded_list_push_back(&udpif->upcalls, &upcall->list_node,
-                                         MAX_QUEUE_LENGTH);
-            if (len > 0) {
-                n_udpif_new_upcalls = len;
-                if (n_udpif_new_upcalls >= FLOW_MISS_MAX_BATCH) {
-                    seq_change(udpif->wait_seq);
-                }
-            } else {
-                COVERAGE_INC(upcall_queue_overflow);
-                upcall_destroy(upcall);
-            }
+            break;
         }
     }
     for (n = 0; n < udpif->n_handlers; ++n) {
-        handler = &udpif->handlers[n];
+        struct handler *handler = &udpif->handlers[n];
+
         if (handler->n_new_upcalls) {
             handler->n_new_upcalls = 0;
             ovs_mutex_lock(&handler->mutex);
diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h
index 9bd19ad..99bbedf 100644
--- a/ofproto/ofproto-dpif-upcall.h
+++ b/ofproto/ofproto-dpif-upcall.h
@@ -40,38 +40,6 @@ void udpif_wait(struct udpif *);
 
 void udpif_revalidate(struct udpif *);
 
-/* udpif can handle some upcalls on its own.  Others need the main ofproto_dpif
- * code to handle them.  This interface passes upcalls not handled by udpif up
- * to the ofproto_dpif main thread. */
-
-/* Type of an upcall. */
-enum upcall_type {
-    /* Handled internally by udpif code.  Not returned by upcall_next().*/
-    BAD_UPCALL,                 /* Some kind of bug somewhere. */
-    MISS_UPCALL,                /* A flow miss.  */
-
-    /* Require main thread's involvement.  May be returned by upcall_next(). */
-    SFLOW_UPCALL,               /* sFlow sample. */
-    FLOW_SAMPLE_UPCALL,         /* Per-flow sampling. */
-    IPFIX_UPCALL                /* Per-bridge sampling. */
-};
-
-/* An upcall. */
-struct upcall {
-    struct list list_node;          /* For queuing upcalls. */
-    struct flow_miss *flow_miss;    /* This upcall's flow_miss. */
-
-    enum upcall_type type;          /* Classification. */
-
-    /* Raw upcall plus data for keeping track of the memory backing it. */
-    struct dpif_upcall dpif_upcall; /* As returned by dpif_recv() */
-    struct ofpbuf upcall_buf;       /* Owns some data in 'dpif_upcall'. */
-    uint64_t upcall_stub[512 / 8];  /* Buffer to reduce need for malloc(). */
-};
-
-struct upcall *upcall_next(struct udpif *);
-void upcall_destroy(struct upcall *);
-
 /* udpif figures out how to forward packets, and does forward them, but it
  * can't set up datapath flows on its own.  This interface passes packet
  * forwarding data from udpif to the higher level ofproto_dpif to allow the
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a5b6814..e5ddc49 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2462,6 +2462,44 @@ xlate_out_copy(struct xlate_out *dst, const struct xlate_out *src)
     ofpbuf_put(&dst->odp_actions, src->odp_actions.data,
                src->odp_actions.size);
 }
+
+/* Returns a reference to the sflow handled associated with ofproto, or NULL if
+ * there is none.  The caller is responsible for decrementing the results ref
+ * count with dpif_sflow_unref(). */
+struct dpif_sflow *
+xlate_get_sflow(const struct ofproto_dpif *ofproto)
+{
+    struct xbridge *xbridge;
+    struct dpif_sflow *sflow;
+
+    ovs_rwlock_rdlock(&xlate_rwlock);
+    xbridge = xbridge_lookup(ofproto);
+    if (xbridge) {
+        sflow = dpif_sflow_ref(xbridge->sflow);
+    }
+    ovs_rwlock_unlock(&xlate_rwlock);
+
+    return sflow;
+}
+
+/* Returns a reference to the ipfix handled associated with ofproto, or NULL if
+ * there is none.  The caller is responsible for decrementing the results ref
+ * count with dpif_ipfix_unref(). */
+struct dpif_ipfix *
+xlate_get_ipfix(const struct ofproto_dpif *ofproto)
+{
+    struct xbridge *xbridge;
+    struct dpif_ipfix *ipfix;
+
+    ovs_rwlock_rdlock(&xlate_rwlock);
+    xbridge = xbridge_lookup(ofproto);
+    if (xbridge) {
+        ipfix = dpif_ipfix_ref(xbridge->ipfix);
+    }
+    ovs_rwlock_unlock(&xlate_rwlock);
+
+    return ipfix;
+}
 
 static struct skb_priority_to_dscp *
 get_skb_priority(const struct xport *xport, uint32_t skb_priority)
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index a54a9e4..8bd669b 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -154,4 +154,9 @@ void xlate_in_init(struct xlate_in *, struct ofproto_dpif *,
 void xlate_out_uninit(struct xlate_out *);
 void xlate_actions_for_side_effects(struct xlate_in *);
 void xlate_out_copy(struct xlate_out *dst, const struct xlate_out *src);
+
+struct dpif_sflow * xlate_get_sflow(const struct ofproto_dpif *)
+    OVS_EXCLUDED(xlate_rwlock);
+struct dpif_ipfix * xlate_get_ipfix(const struct ofproto_dpif *)
+    OVS_EXCLUDED(xlate_rwlock);
 #endif /* ofproto-dpif-xlate.h */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 80874b8..d4d0ad6 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3428,107 +3428,12 @@ handle_flow_misses(struct dpif_backer *backer, struct flow_miss_batch *fmb)
 }
 
 static void
-handle_sflow_upcall(struct dpif_backer *backer,
-                    const struct dpif_upcall *upcall)
-{
-    struct ofproto_dpif *ofproto;
-    union user_action_cookie cookie;
-    struct flow flow;
-    odp_port_t odp_in_port;
-
-    if (xlate_receive(backer, upcall->packet, upcall->key, upcall->key_len,
-                      &flow, NULL, &ofproto, &odp_in_port)
-        || !ofproto->sflow) {
-        return;
-    }
-
-    memset(&cookie, 0, sizeof cookie);
-    memcpy(&cookie, nl_attr_get(upcall->userdata), sizeof cookie.sflow);
-    dpif_sflow_received(ofproto->sflow, upcall->packet, &flow,
-                        odp_in_port, &cookie);
-}
-
-static void
-handle_flow_sample_upcall(struct dpif_backer *backer,
-                          const struct dpif_upcall *upcall)
-{
-    struct ofproto_dpif *ofproto;
-    union user_action_cookie cookie;
-    struct flow flow;
-
-    if (xlate_receive(backer, upcall->packet, upcall->key, upcall->key_len,
-                      &flow, NULL, &ofproto, NULL)
-        || !ofproto->ipfix) {
-        return;
-    }
-
-    memset(&cookie, 0, sizeof cookie);
-    memcpy(&cookie, nl_attr_get(upcall->userdata), sizeof cookie.flow_sample);
-
-    /* The flow reflects exactly the contents of the packet.  Sample
-     * the packet using it. */
-    dpif_ipfix_flow_sample(ofproto->ipfix, upcall->packet, &flow,
-                           cookie.flow_sample.collector_set_id,
-                           cookie.flow_sample.probability,
-                           cookie.flow_sample.obs_domain_id,
-                           cookie.flow_sample.obs_point_id);
-}
-
-static void
-handle_ipfix_upcall(struct dpif_backer *backer,
-                    const struct dpif_upcall *upcall)
-{
-    struct ofproto_dpif *ofproto;
-    struct flow flow;
-
-    if (xlate_receive(backer, upcall->packet, upcall->key, upcall->key_len,
-                      &flow, NULL, &ofproto, NULL)
-        || !ofproto->ipfix) {
-        return;
-    }
-
-    /* The flow reflects exactly the contents of the packet.  Sample
-     * the packet using it. */
-    dpif_ipfix_bridge_sample(ofproto->ipfix, upcall->packet, &flow);
-}
-
-static void
 handle_upcalls(struct dpif_backer *backer)
 {
     struct flow_miss_batch *fmb;
     int n_processed;
 
     for (n_processed = 0; n_processed < FLOW_MISS_MAX_BATCH; n_processed++) {
-        struct upcall *upcall = upcall_next(backer->udpif);
-
-        if (!upcall) {
-            break;
-        }
-
-        switch (upcall->type) {
-        case SFLOW_UPCALL:
-            handle_sflow_upcall(backer, &upcall->dpif_upcall);
-            break;
-
-        case FLOW_SAMPLE_UPCALL:
-            handle_flow_sample_upcall(backer, &upcall->dpif_upcall);
-            break;
-
-        case IPFIX_UPCALL:
-            handle_ipfix_upcall(backer, &upcall->dpif_upcall);
-            break;
-
-        case BAD_UPCALL:
-            break;
-
-        case MISS_UPCALL:
-            NOT_REACHED();
-        }
-
-        upcall_destroy(upcall);
-    }
-
-    for (n_processed = 0; n_processed < FLOW_MISS_MAX_BATCH; n_processed++) {
         struct drop_key *drop_key = drop_key_next(backer->udpif);
         if (!drop_key) {
             break;
-- 
1.7.9.5




More information about the dev mailing list