[ovs-dev] [DPDK Upcalls 07/11] xlate: Handle VLAN splinters in xlate_actions().

Ethan Jackson ethan at nicira.com
Sat Aug 2 01:39:18 UTC 2014


I find this quite a bit simpler to reason about.  Also, future patches
require xlate_actions() not to modify the packet.

Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
 ofproto/ofproto-dpif-upcall.c | 25 ++------------
 ofproto/ofproto-dpif-xlate.c  | 80 +++++++++++++++++++++++++------------------
 ofproto/ofproto-dpif-xlate.h  |  9 +++--
 ofproto/ofproto-dpif.c        |  6 ++--
 tests/vlan-splinters.at       |  4 +--
 5 files changed, 58 insertions(+), 66 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 48393b0..b0fd203 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -858,7 +858,7 @@ convert_upcall(struct udpif *udpif, struct upcall *upcall)
     odp_port_t odp_in_port;
     int error;
 
-    error = xlate_receive(udpif->backer, packet, dupcall->key,
+    error = xlate_receive(udpif->backer, dupcall->key,
                           dupcall->key_len, &flow,
                           &ofproto, &ipfix, &sflow, NULL, &odp_in_port);
 
@@ -967,25 +967,6 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
 
         fail_open = fail_open || upcall->xout.fail_open;
 
-        if (upcall->flow.in_port.ofp_port
-            != vsp_realdev_to_vlandev(upcall->ofproto,
-                                      upcall->flow.in_port.ofp_port,
-                                      upcall->flow.vlan_tci)) {
-            /* This packet was received on a VLAN splinter port.  We
-             * added a VLAN to the packet to make the packet resemble
-             * the flow, but the actions were composed assuming that
-             * the packet contained no VLAN.  So, we must remove the
-             * VLAN header from the packet before trying to execute the
-             * actions. */
-            if (ofpbuf_size(upcall->xout.odp_actions)) {
-                eth_pop_vlan(packet);
-            }
-
-            /* Remove the flow vlan tags inserted by vlan splinter logic
-             * to ensure megaflow masks generated match the data path flow. */
-            upcall->flow.vlan_tci = 0;
-        }
-
         /* Do not install a flow into the datapath if:
          *
          *    - The datapath already has too many flows.
@@ -1257,7 +1238,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
         goto exit;
     }
 
-    error = xlate_receive(udpif->backer, NULL, ukey->key, ukey->key_len, &flow,
+    error = xlate_receive(udpif->backer, ukey->key, ukey->key_len, &flow,
                           &ofproto, NULL, NULL, &netflow, &odp_in_port);
     if (error) {
         goto exit;
@@ -1385,7 +1366,7 @@ push_dump_ops__(struct udpif *udpif, struct dump_op *ops, size_t n_ops)
             }
             ovs_mutex_unlock(&op->ukey->mutex);
 
-            error = xlate_receive(udpif->backer, NULL, op->op.u.flow_del.key,
+            error = xlate_receive(udpif->backer, op->op.u.flow_del.key,
                                   op->op.u.flow_del.key_len, &flow, &ofproto,
                                   NULL, NULL, &netflow, NULL);
             if (!error) {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index c816135..f115849 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -934,23 +934,16 @@ xlate_ofport_remove(struct ofport_dpif *ofport)
     xlate_xport_remove(new_xcfg, xport);
 }
 
-/* Given a datpath, packet, and flow metadata ('backer', 'packet', and 'key'
- * respectively), populates 'flow' with the result of odp_flow_key_to_flow().
- * Optionally populates 'ofproto' with the ofproto_dpif, 'odp_in_port' with
- * the datapath in_port, that 'packet' ingressed, and 'ipfix', 'sflow', and
- * 'netflow' with the appropriate handles for those protocols if they're
- * enabled.  Caller is responsible for unrefing them.
+/* Given a datpath and flow metadata ('backer', and 'key' respectively),
+ * populates 'flow' with the result of odp_flow_key_to_flow().  Optionally
+ * populates 'ofproto' with the ofproto_dpif, 'odp_in_port' with the datapath
+ * in_port, and 'ipfix', 'sflow', and 'netflow' with the appropriate handles
+ * for those protocols if they're enabled.  Caller is responsible for unrefing
+ * them.
  *
  * If 'ofproto' is nonnull, requires 'flow''s in_port to exist.  Otherwise sets
  * 'flow''s in_port to OFPP_NONE.
  *
- * This function does post-processing on data returned from
- * odp_flow_key_to_flow() to help make VLAN splinters transparent to the rest
- * of the upcall processing logic.  In particular, if the extracted in_port is
- * a VLAN splinter port, it replaces flow->in_port by the "real" port, sets
- * flow->vlan_tci correctly for the VLAN of the VLAN splinter port, and pushes
- * a VLAN header onto 'packet' (if it is nonnull).
- *
  * Similarly, this function also includes some logic to help with tunnels.  It
  * may modify 'flow' as necessary to make the tunneling implementation
  * transparent to the upcall processing logic.
@@ -958,19 +951,16 @@ xlate_ofport_remove(struct ofport_dpif *ofport)
  * Returns 0 if successful, ENODEV if the parsed flow has no associated ofport,
  * or some other positive errno if there are other problems. */
 int
-xlate_receive(const struct dpif_backer *backer, struct ofpbuf *packet,
-              const struct nlattr *key, size_t key_len, struct flow *flow,
-              struct ofproto_dpif **ofproto, struct dpif_ipfix **ipfix,
-              struct dpif_sflow **sflow, struct netflow **netflow,
-              odp_port_t *odp_in_port)
+xlate_receive(const struct dpif_backer *backer, const struct nlattr *key,
+              size_t key_len, struct flow *flow, struct ofproto_dpif **ofproto,
+              struct dpif_ipfix **ipfix, struct dpif_sflow **sflow,
+              struct netflow **netflow, odp_port_t *odp_in_port)
 {
     struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
-    int error = ENODEV;
     const struct xport *xport;
 
     if (odp_flow_key_to_flow(key, key_len, flow) == ODP_FIT_ERROR) {
-        error = EINVAL;
-        return error;
+        return EINVAL;
     }
 
     if (odp_in_port) {
@@ -983,19 +973,8 @@ xlate_receive(const struct dpif_backer *backer, struct ofpbuf *packet,
 
     flow->in_port.ofp_port = xport ? xport->ofp_port : OFPP_NONE;
     if (!xport) {
-        return error;
-    }
-
-    if (vsp_adjust_flow(xport->xbridge->ofproto, flow)) {
-        if (packet) {
-            /* Make the packet resemble the flow, so that it gets sent to
-             * an OpenFlow controller properly, so that it looks correct
-             * for sFlow, and so that flow_extract() will get the correct
-             * vlan_tci if it is called on 'packet'. */
-            eth_push_vlan(packet, htons(ETH_TYPE_VLAN), flow->vlan_tci);
-        }
+        return ENODEV;
     }
-    error = 0;
 
     if (ofproto) {
         *ofproto = xport->xbridge->ofproto;
@@ -1012,7 +991,7 @@ xlate_receive(const struct dpif_backer *backer, struct ofpbuf *packet,
     if (netflow) {
         *netflow = netflow_ref(xport->xbridge->netflow);
     }
-    return error;
+    return 0;
 }
 
 static struct xbridge *
@@ -3918,6 +3897,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     struct flow orig_flow;
     struct xlate_ctx ctx;
     size_t ofpacts_len;
+    bool vsp_adjusted;
     bool tnl_may_send;
     bool is_icmp;
 
@@ -3995,6 +3975,27 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     ctx.use_recirc = false;
     ctx.was_mpls = false;
 
+    vsp_adjusted = vsp_adjust_flow(ctx.xbridge->ofproto, flow);
+    if (vsp_adjusted) {
+        if (xin->report_hook) {
+            struct ds ds = DS_EMPTY_INITIALIZER;
+
+            ds_put_cstr(&ds, "Adjusted for VLAN splinters: ");
+            flow_format(&ds, flow);
+            xlate_report(&ctx, ds_cstr(&ds));
+            ds_destroy(&ds);
+        }
+
+        if (xin->packet) {
+            /* Make the packet resemble the flow, so that it gets sent to
+             * an OpenFlow controller properly, so that it looks correct
+             * for sFlow, and so that flow_extract() will get the correct
+             * vlan_tci if it is called on 'packet'. */
+            eth_push_vlan(CONST_CAST(struct ofpbuf *, xin->packet),
+                          htons(ETH_TYPE_VLAN), flow->vlan_tci);
+        }
+    }
+
     if (!xin->ofpacts && !ctx.rule) {
         ctx.table_id = rule_dpif_lookup(ctx.xbridge->ofproto, flow,
                                         !xin->skip_wildcards ? wc : NULL,
@@ -4187,6 +4188,17 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         wc->masks.tp_src &= htons(UINT8_MAX);
         wc->masks.tp_dst &= htons(UINT8_MAX);
     }
+
+    if (vsp_adjusted) {
+        /* This packet was received on a VLAN splinter port.  We added a VLAN
+         * to the packet to make the packet resemble the flow, but the actions
+         * were composed assuming that the packet contained no VLAN.  So, we
+         * must remove the VLAN header from the packet before trying to execute
+         * the actions. */
+        if (xin->packet) {
+            eth_pop_vlan(CONST_CAST(struct ofpbuf *, xin->packet));
+        }
+    }
 }
 
 /* Sends 'packet' out 'ofport'.
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 3f6807d..3522c0b 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -173,11 +173,10 @@ void xlate_ofport_set(struct ofproto_dpif *, struct ofbundle *,
                       bool may_enable);
 void xlate_ofport_remove(struct ofport_dpif *);
 
-int xlate_receive(const struct dpif_backer *, struct ofpbuf *packet,
-                  const struct nlattr *key, size_t key_len,
-                  struct flow *, struct ofproto_dpif **, struct dpif_ipfix **,
-                  struct dpif_sflow **, struct netflow **,
-                  odp_port_t *odp_in_port);
+int xlate_receive(const struct dpif_backer *, const struct nlattr *key,
+                  size_t key_len, struct flow *, struct ofproto_dpif **,
+                  struct dpif_ipfix **, struct dpif_sflow **,
+                  struct netflow **, odp_port_t *odp_in_port);
 
 void xlate_actions(struct xlate_in *, struct xlate_out *);
 void xlate_in_init(struct xlate_in *, struct ofproto_dpif *,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 161b26a..c49f1fb 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4205,7 +4205,7 @@ parse_flow_and_packet(int argc, const char *argv[],
             goto exit;
         }
 
-        if (xlate_receive(backer, NULL, ofpbuf_data(&odp_key),
+        if (xlate_receive(backer, ofpbuf_data(&odp_key),
                           ofpbuf_size(&odp_key), flow,
                           ofprotop, NULL, NULL, NULL, NULL)) {
             error = "Invalid datapath flow";
@@ -4592,8 +4592,8 @@ ofproto_dpif_contains_flow(const struct ofproto_dpif *ofproto,
     struct ofproto_dpif *ofp;
     struct flow flow;
 
-    xlate_receive(ofproto->backer, NULL, key, key_len, &flow, &ofp,
-                  NULL, NULL, NULL, NULL);
+    xlate_receive(ofproto->backer, key, key_len, &flow, &ofp, NULL, NULL, NULL,
+                  NULL);
     return ofp == ofproto;
 }
 
diff --git a/tests/vlan-splinters.at b/tests/vlan-splinters.at
index b38ab52..6406029 100644
--- a/tests/vlan-splinters.at
+++ b/tests/vlan-splinters.at
@@ -30,8 +30,8 @@ for args in '9 p2' '11 p3' '15 p4'; do
     # treated as if it had been received on p1 in the correct VLAN.
     AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port($splinter_port)"],
              [0], [stdout])
-    AT_CHECK_UNQUOTED([sed -n '/^Flow/p; /^Datapath/p' stdout], [0], [dnl
-Flow: metadata=0,in_port=$p1,dl_vlan=$vlan,dl_vlan_pcp=0,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x05ff
+    AT_CHECK_UNQUOTED([sed -n '/^Adjusted/p; /^Datapath/p' stdout], [0], [dnl
+Adjusted for VLAN splinters: metadata=0,in_port=$p1,dl_vlan=$vlan,dl_vlan_pcp=0,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x05ff
 Datapath actions: $access_port
 ])
 
-- 
1.8.1.2




More information about the dev mailing list