]> git.proxmox.com Git - mirror_ovs.git/commitdiff
conntrack: Add 'dl_type' parameter to conntrack_execute().
authorDaniele Di Proietto <diproiettod@vmware.com>
Thu, 26 May 2016 01:10:09 +0000 (18:10 -0700)
committerDaniele Di Proietto <diproiettod@vmware.com>
Thu, 28 Jul 2016 01:53:29 +0000 (18:53 -0700)
Now that dpif_execute has a 'flow' member, it's pretty easy to access a
the flow (or the matching megaflow) in dp_execute_cb().

This means that's not necessary anymore for the connection tracker to
reextract 'dl_type' from the packet, it can be passed as a parameter.

This change means that we have to complicate sightly test-conntrack to
group the packets by dl_type before passing them to the connection
tracker.

Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
Acked-by: Joe Stringer <joe@ovn.org>
lib/conntrack.c
lib/conntrack.h
lib/dpif-netdev.c
tests/test-conntrack.c

index 595a63710e25694a3c7cd8f15893ff2ff255ce00..0c52e9849cb6cc6cc5bc1f5731e002e7755e3bf0 100644 (file)
@@ -53,7 +53,8 @@ struct conn_lookup_ctx {
 };
 
 static bool conn_key_extract(struct conntrack *, struct dp_packet *,
-                             struct conn_lookup_ctx *, uint16_t zone);
+                             ovs_be16 dl_type, struct conn_lookup_ctx *,
+                             uint16_t zone);
 static uint32_t conn_key_hash(const struct conn_key *, uint32_t basis);
 static void conn_key_reverse(struct conn_key *);
 static void conn_key_lookup(struct conntrack_bucket *ctb,
@@ -265,7 +266,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
  * 'setlabel' behaves similarly for the connection label.*/
 int
 conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
-                  bool commit, uint16_t zone, const uint32_t *setmark,
+                  ovs_be16 dl_type, bool commit, uint16_t zone,
+                  const uint32_t *setmark,
                   const struct ovs_key_ct_labels *setlabel,
                   const char *helper)
 {
@@ -299,7 +301,7 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
     for (i = 0; i < cnt; i++) {
         unsigned bucket;
 
-        if (!conn_key_extract(ct, pkts[i], &ctxs[i], zone)) {
+        if (!conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone)) {
             write_ct_md(pkts[i], CS_INVALID, zone, 0, OVS_U128_ZERO);
             continue;
         }
@@ -915,7 +917,7 @@ extract_l4(struct conn_key *key, const void *data, size_t size, bool *related,
 }
 
 static bool
-conn_key_extract(struct conntrack *ct, struct dp_packet *pkt,
+conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
                  struct conn_lookup_ctx *ctx, uint16_t zone)
 {
     const struct eth_header *l2 = dp_packet_l2(pkt);
@@ -939,43 +941,32 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt,
      *    We already have the l3 and l4 headers' pointers.  Extracting
      *    the l3 addresses and the l4 ports is really cheap, since they
      *    can be found at fixed locations.
-     * 2) To extract the l3 and l4 types.
-     *    Extracting the l3 and l4 types (especially the l3[1]) on the
-     *    other hand is quite expensive, because they're not at a
-     *    fixed location.
+     * 2) To extract the l4 type.
+     *    Extracting the l4 types, for IPv6 can be quite expensive, because
+     *    it's not at a fixed location.
      *
      * Here's a way to avoid (2) with the help of the datapath.
-     * The datapath doesn't keep the packet's extracted flow[2], so
+     * The datapath doesn't keep the packet's extracted flow[1], so
      * using that is not an option.  We could use the packet's matching
-     * megaflow for l3 type (it's always unwildcarded), and for l4 type
-     * (we have to unwildcard it first).  This means either:
+     * megaflow, but we have to make sure that the l4 type (nw_proto)
+     * is unwildcarded.  This means either:
      *
-     * a) dpif-netdev passes the matching megaflow to dp_execute_cb(), which
-     *    is used to extract the l3 type.  Unfortunately, dp_execute_cb() is
-     *    used also in dpif_netdev_execute(), which doesn't have a matching
-     *    megaflow.
+     * a) dpif-netdev unwildcards the l4 type when a new flow is installed
+     *    if the actions contains ct().
      *
-     * b) We define an alternative OVS_ACTION_ATTR_CT, used only by the
-     *    userspace datapath, which includes l3 (and l4) type.  The
-     *    alternative action could be generated by ofproto-dpif specifically
-     *    for the userspace datapath. Having a different interface for
-     *    userspace and kernel doesn't seem very clean, though.
+     * b) ofproto-dpif-xlate unwildcards the l4 type when translating a ct()
+     *    action.  This is already done in different actions, but it's
+     *    unnecessary for the kernel.
      *
      * ---
-     * [1] A simple benchmark (running only the connection tracker
-     *     over and over on the same packets) shows that if the
-     *     l3 type is already provided we are 15% faster (running the
-     *     connection tracker over a couple of DPDK devices with a
-     *     stream of UDP 64-bytes packets shows that we are 4% faster).
-     *
-     * [2] The reasons for this are that keeping the flow increases
+     * [1] The reasons for this are that keeping the flow increases
      *     (slightly) the cache footprint and increases computation
      *     time as we move the packet around. Most importantly, the flow
      *     should be updated by the actions and this can be slow, as
      *     we use a sparse representation (miniflow).
      *
      */
-    ctx->key.dl_type = parse_dl_type(l2, (char *) l3 - (char *) l2);
+    ctx->key.dl_type = dl_type;
     if (ctx->key.dl_type == htons(ETH_TYPE_IP)) {
         ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL, true);
     } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
index d188105ec33cb7d537bc2e4db3702b5e60508073..254f61c14afa0eb2c87f4f9d7200cd47e1e45685 100644 (file)
@@ -65,7 +65,8 @@ void conntrack_init(struct conntrack *);
 void conntrack_destroy(struct conntrack *);
 
 int conntrack_execute(struct conntrack *, struct dp_packet_batch *,
-                      bool commit, uint16_t zone, const uint32_t *setmark,
+                      ovs_be16 dl_type, bool commit,
+                      uint16_t zone, const uint32_t *setmark,
                       const struct ovs_key_ct_labels *setlabel,
                       const char *helper);
 
index 166667f06b8dc8e829538b36ff93fe8512a64333..828171e73ec315886f7f6f7fda0f7e7186d7363b 100644 (file)
@@ -527,7 +527,7 @@ static int dpif_netdev_open(const struct dpif_class *, const char *name,
                             bool create, struct dpif **);
 static void dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd,
                                       struct dp_packet_batch *,
-                                      bool may_steal,
+                                      bool may_steal, const struct flow *flow,
                                       const struct nlattr *actions,
                                       size_t actions_len,
                                       long long now);
@@ -2553,8 +2553,9 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
     }
 
     packet_batch_init_packet(&pp, execute->packet);
-    dp_netdev_execute_actions(pmd, &pp, false, execute->actions,
-                              execute->actions_len, time_msec());
+    dp_netdev_execute_actions(pmd, &pp, false, execute->flow,
+                              execute->actions, execute->actions_len,
+                              time_msec());
 
     if (pmd->core_id == NON_PMD_CORE_ID) {
         ovs_mutex_unlock(&dp->non_pmd_mutex);
@@ -3863,7 +3864,7 @@ packet_batch_per_flow_execute(struct packet_batch_per_flow *batch,
 
     actions = dp_netdev_flow_get_actions(flow);
 
-    dp_netdev_execute_actions(pmd, &batch->array, true,
+    dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow,
                               actions->actions, actions->size, now);
 }
 
@@ -3990,7 +3991,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet,
      * the actions.  Otherwise, if there are any slow path actions,
      * we'll send the packet up twice. */
     packet_batch_init_packet(&b, packet);
-    dp_netdev_execute_actions(pmd, &b, true,
+    dp_netdev_execute_actions(pmd, &b, true, &match.flow,
                               actions->data, actions->size, now);
 
     add_actions = put_actions->size ? put_actions : actions;
@@ -4162,6 +4163,7 @@ dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd,
 struct dp_netdev_execute_aux {
     struct dp_netdev_pmd_thread *pmd;
     long long now;
+    const struct flow *flow;
 };
 
 static void
@@ -4302,7 +4304,7 @@ dp_execute_userspace_action(struct dp_netdev_pmd_thread *pmd,
                              NULL);
     if (!error || error == ENOSPC) {
         packet_batch_init_packet(&b, packet);
-        dp_netdev_execute_actions(pmd, &b, may_steal,
+        dp_netdev_execute_actions(pmd, &b, may_steal, flow,
                                   actions->data, actions->size, now);
     } else if (may_steal) {
         dp_packet_delete(packet);
@@ -4505,8 +4507,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
             }
         }
 
-        conntrack_execute(&dp->conntrack, packets_, commit, zone,
-                          setmark, setlabel, helper);
+        conntrack_execute(&dp->conntrack, packets_, aux->flow->dl_type, commit,
+                          zone, setmark, setlabel, helper);
         break;
     }
 
@@ -4530,11 +4532,11 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
 static void
 dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd,
                           struct dp_packet_batch *packets,
-                          bool may_steal,
+                          bool may_steal, const struct flow *flow,
                           const struct nlattr *actions, size_t actions_len,
                           long long now)
 {
-    struct dp_netdev_execute_aux aux = { pmd, now };
+    struct dp_netdev_execute_aux aux = { pmd, now, flow };
 
     odp_execute_actions(&aux, packets, may_steal, actions,
                         actions_len, dp_execute_cb);
index 0ff70d1a81b1e9ec21fd379d7c9855443bc99321..6c1b3735c9fc2172f0c7ff8bc56f9f3593fb837f 100644 (file)
@@ -30,7 +30,7 @@ static const char payload[] = "50540000000a50540000000908004500001c0000000000"
                               "11a4cd0a0101010a0101020001000200080000";
 
 static struct dp_packet_batch *
-prepare_packets(size_t n, bool change, unsigned tid)
+prepare_packets(size_t n, bool change, unsigned tid, ovs_be16 *dl_type)
 {
     struct dp_packet_batch *pkt_batch = xzalloc(sizeof *pkt_batch);
     struct flow flow;
@@ -56,8 +56,10 @@ prepare_packets(size_t n, bool change, unsigned tid)
         }
 
         pkt_batch->packets[i] = pkt;
+        *dl_type = flow.dl_type;
     }
 
+
     return pkt_batch;
 }
 
@@ -83,12 +85,13 @@ ct_thread_main(void *aux_)
 {
     struct thread_aux *aux = aux_;
     struct dp_packet_batch *pkt_batch;
+    ovs_be16 dl_type;
     size_t i;
 
-    pkt_batch = prepare_packets(batch_size, change_conn, aux->tid);
+    pkt_batch = prepare_packets(batch_size, change_conn, aux->tid, &dl_type);
     ovs_barrier_block(&barrier);
     for (i = 0; i < n_pkts; i += batch_size) {
-        conntrack_execute(&ct, pkt_batch, true, 0, NULL, NULL, NULL);
+        conntrack_execute(&ct, pkt_batch, dl_type, true, 0, NULL, NULL, NULL);
     }
     ovs_barrier_block(&barrier);
     destroy_packets(pkt_batch);
@@ -147,6 +150,44 @@ test_benchmark(struct ovs_cmdl_context *ctx)
     free(threads);
 }
 
+static void
+pcap_batch_execute_conntrack(struct conntrack *ct,
+                             struct dp_packet_batch *pkt_batch)
+{
+    size_t i;
+    struct dp_packet_batch new_batch;
+    ovs_be16 dl_type = htons(0);
+
+    dp_packet_batch_init(&new_batch);
+
+    /* pkt_batch contains packets with different 'dl_type'. We have to
+     * call conntrack_execute() on packets with the same 'dl_type'. */
+
+    for (i = 0; i < pkt_batch->count; i++) {
+        struct dp_packet *pkt = pkt_batch->packets[i];
+        struct flow flow;
+
+        /* This also initializes the l3 and l4 pointers. */
+        flow_extract(pkt, &flow);
+
+        if (!new_batch.count) {
+            dl_type = flow.dl_type;
+        }
+
+        if (flow.dl_type != dl_type) {
+            conntrack_execute(ct, &new_batch, dl_type, true, 0, NULL, NULL,
+                              NULL);
+            dp_packet_batch_init(&new_batch);
+        }
+        new_batch.packets[new_batch.count++] = pkt;
+    }
+
+    if (new_batch.count) {
+        conntrack_execute(ct, &new_batch, dl_type, true, 0, NULL, NULL, NULL);
+    }
+
+}
+
 static void
 test_pcap(struct ovs_cmdl_context *ctx)
 {
@@ -179,13 +220,10 @@ test_pcap(struct ovs_cmdl_context *ctx)
         dp_packet_batch_init(&pkt_batch);
 
         for (i = 0; i < batch_size; i++) {
-            struct flow dummy_flow;
-
             err = ovs_pcap_read(pcap, &pkt_batch.packets[i], NULL);
             if (err) {
                 break;
             }
-            flow_extract(pkt_batch.packets[i], &dummy_flow);
         }
 
         pkt_batch.count = i;
@@ -193,7 +231,7 @@ test_pcap(struct ovs_cmdl_context *ctx)
             break;
         }
 
-        conntrack_execute(&ct, &pkt_batch, true, 0, NULL, NULL, NULL);
+        pcap_batch_execute_conntrack(&ct, &pkt_batch);
 
         for (i = 0; i < pkt_batch.count; i++) {
             struct ds ds = DS_EMPTY_INITIALIZER;