[ovs-dev,V14,1/4] Flow and action changes for 802.1AD
diff mbox

Message ID 1443821521-14770-2-git-send-email-thomasfherbert@gmail.com
State RFC
Headers show

Commit Message

Thomas F Herbert Oct. 2, 2015, 9:31 p.m. UTC
From: "Thomas F. Herbert" <thomasfherbert@gmail.com>

ToDo: The flow structure needs to be updated to hold both the inner and outer
tpid since these are encoded by the kernel module.

The flow structure is updated to hold the customer tci.
Flow key extraction is changed to add support for ctci and both TPIDs.
Parsing to support pushing and popping with both single and double
tagged vlans. In response to reviewers comments on V6, all changes
affected by the flow structure are gathered in this patch.

Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
---
 lib/flow.c                   |  2 +-
 lib/flow.h                   | 12 +++++++++---
 lib/ofp-actions.c            | 32 +++++++++++++++++++-------------
 lib/ofp-actions.h            | 14 +++++++++++---
 ofproto/ofproto-dpif-xlate.c | 11 +++++++++++
 5 files changed, 51 insertions(+), 20 deletions(-)

Comments

Ben Pfaff Nov. 25, 2015, 5:54 a.m. UTC | #1
On Mon, Nov 09, 2015 at 08:30:19PM -0800, Thomas F Herbert wrote:
> 
> 
> On 11/9/15 12:58 PM, Ben Pfaff wrote:
> >On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
> >>From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
> >>
> >>ToDo: The flow structure needs to be updated to hold both the inner and outer
> >>tpid since these are encoded by the kernel module.
> >I think I'd like to see the version with both TPIDs encoded.  That's the
> >way I've always envisioned this working.
> From an architectural view, it sounds indeed quite sensible. Also, it would
> get rid of the vlan depth calculation in the above code which is working but
> ugly. The $10 question is: are you implying that we should change the ABI by
> adding new OVS_KEY_ATTRIBs for the two TPIDs? This would raise backward
> compatibility and ABI issues.

I think that we covered this in-person at ovscon last week, but just in
case...  I don't think that this requires a kernel ABI change, because
the kernel ABI has always included the TPID.  I'm only suggesting that
"struct flow" consistently include the TPID; struct flow always goes
through some kind of translation whenever it passes out of OVS
userspace, so that shouldn't in itself break anything.

> >Also, once we have the TPIDs in place in struct flow, I am not sure
> >whether it is worthwhile to keep adding the VLAN_CFI bit to the TCI
> >value to indicate that a header is present.
> I assume you are suggesting that we determining a VLAN is present by
> checking eth_type_vlan(). With this we could allow any value for the VLAN
> including a 1 in the DEI bit. Maybe this should be a separate patch to
> eliminate the redundancy in openvswitch once the above change was made.

Maybe an example would help.  Suppose we define:
        struct flow_vlan {
            ovs_be32 tpid;
            ovs_be32 tci;
        };

and then in struct flow replace vlan_tci by "struct flow_vlan
vlans[2];".  (I'm not saying this is the way to go but it's a
possibility.)

Then, we know that there's at least one VLAN in 'flow' if
'flow->vlans[0].tpid != 0' and that there are two VLANs in 'flow' if
'flow->vlans[1].tpid != 0' (presumably they'd be left-justified so you
wouldn't get just the latter).
Thomas F Herbert Nov. 25, 2015, 7 p.m. UTC | #2
On 11/25/15 12:54 AM, Ben Pfaff wrote:
> On Mon, Nov 09, 2015 at 08:30:19PM -0800, Thomas F Herbert wrote:
>>
>> On 11/9/15 12:58 PM, Ben Pfaff wrote:
>>> On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
>>>> From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
>>>>
>>>> ToDo: The flow structure needs to be updated to hold both the inner and outer
>>>> tpid since these are encoded by the kernel module.
>>> I think I'd like to see the version with both TPIDs encoded.  That's the
>>> way I've always envisioned this working.
>>  From an architectural view, it sounds indeed quite sensible. Also, it would
>> get rid of the vlan depth calculation in the above code which is working but
>> ugly. The $10 question is: are you implying that we should change the ABI by
>> adding new OVS_KEY_ATTRIBs for the two TPIDs? This would raise backward
>> compatibility and ABI issues.
> I think that we covered this in-person at ovscon last week, but just in
> case...  I don't think that this requires a kernel ABI change, because
> the kernel ABI has always included the TPID.  I'm only suggesting that
> "struct flow" consistently include the TPID; struct flow always goes
> through some kind of translation whenever it passes out of OVS
> userspace, so that shouldn't in itself break anything.
Yes, thanks for the comment. I will update patch to encode and decode 
both tpid's.
>
>>> Also, once we have the TPIDs in place in struct flow, I am not sure
>>> whether it is worthwhile to keep adding the VLAN_CFI bit to the TCI
>>> value to indicate that a header is present.
>> I assume you are suggesting that we determining a VLAN is present by
>> checking eth_type_vlan(). With this we could allow any value for the VLAN
>> including a 1 in the DEI bit. Maybe this should be a separate patch to
>> eliminate the redundancy in openvswitch once the above change was made.
There was a separate comment by Pravin discussing eliminating the DEI. 
Now that we are encoding tpid's explicitly we could check tpid ethertype 
to determine if a vlan is present but probably that should wait for 
another patch.
> Maybe an example would help.  Suppose we define:
>          struct flow_vlan {
>              ovs_be32 tpid;
>              ovs_be32 tci;
>          };
Yes, I did this in the kernel patch.
>
> and then in struct flow replace vlan_tci by "struct flow_vlan
> vlans[2];".  (I'm not saying this is the way to go but it's a
> possibility.)
>
> Then, we know that there's at least one VLAN in 'flow' if
> 'flow->vlans[0].tpid != 0' and that there are two VLANs in 'flow' if
> 'flow->vlans[1].tpid != 0' (presumably they'd be left-justified so you
> wouldn't get just the latter).
Yes, this will make for cleaner code to calculate depth of encapsulation 
vlan.   I think that since vlans are never nested more then 2 deep, I 
could encode it the same way I do in the kernel flow key with two 
structs instead of an array of structs.
Ben Pfaff Jan. 4, 2016, 6:20 p.m. UTC | #3
On Wed, Dec 30, 2015 at 07:29:15PM -0500, Thomas F Herbert wrote:
> 
> 
> On 11/9/15 3:58 PM, Ben Pfaff wrote:
> >On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
> >>From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
> >>
> >>ToDo: The flow structure needs to be updated to hold both the inner and outer
> >>tpid since these are encoded by the kernel module.
> >I think I'd like to see the version with both TPIDs encoded.  That's the
> >way I've always envisioned this working.
> I have been working on the changes to add the inner and outer tpids to the
> flow struct this week. I changed flow struct to place two vlan structs right
> after the dl_type with the outer first followed by the inner. This is done
> so the miniflow would still work with few modifications because dl_type and
> outer tci are sequential. However, this raises the issue as to whether the
> outer tpid and the inner vlan, tci and tpid should also be added to the
> miniflow. What are your thoughts?

I don't understand the question.  A miniflow and a flow represent the
same data, only a miniflow has a compressed format.
Ben Pfaff Jan. 4, 2016, 8:30 p.m. UTC | #4
On Mon, Jan 04, 2016 at 02:46:09PM -0500, Thomas F Herbert wrote:
> 
> 
> On 1/4/16 1:20 PM, Ben Pfaff wrote:
> >On Wed, Dec 30, 2015 at 07:29:15PM -0500, Thomas F Herbert wrote:
> >>
> >>On 11/9/15 3:58 PM, Ben Pfaff wrote:
> >>>On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
> >>>>From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
> >>>>
> >>>>ToDo: The flow structure needs to be updated to hold both the inner and outer
> >>>>tpid since these are encoded by the kernel module.
> >>>I think I'd like to see the version with both TPIDs encoded.  That's the
> >>>way I've always envisioned this working.
> >>I have been working on the changes to add the inner and outer tpids to the
> >>flow struct this week. I changed flow struct to place two vlan structs right
> >>after the dl_type with the outer first followed by the inner. This is done
> >>so the miniflow would still work with few modifications because dl_type and
> >>outer tci are sequential. However, this raises the issue as to whether the
> >>outer tpid and the inner vlan, tci and tpid should also be added to the
> >>miniflow. What are your thoughts?
> >I don't understand the question.  A miniflow and a flow represent the
> >same data, only a miniflow has a compressed format.
> Yes, they represent or should represent the same data but that wasn't my
> question. Maybe the question will be clearer when I submit the patch. The
> miniflow_extract function requires some changes that wasn't done for
> previous versions of this patch. The current miniflow extract compresses
> only the outer vlan_tci, dl_type. For this version, I am writing a
> miniflow_push_vlan() macro to compress both the tpid and tci into the
> miniflow and call the same function to compress the inner tpid and tci as
> well.

OK.  I think it will be clearer from the patch.

Patch
diff mbox

diff --git a/lib/flow.c b/lib/flow.c
index 84048e8..f90e833 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -302,7 +302,7 @@  parse_vlan(const void **datap, size_t *sizep)
 
     data_pull(datap, sizep, ETH_ADDR_LEN * 2);
 
-    if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
+    if (eth_type_vlan(eth->eth_type)) {
         if (OVS_LIKELY(*sizep
                        >= sizeof(struct qtag_prefix) + sizeof(ovs_be16))) {
             const struct qtag_prefix *qp = data_pull(datap, sizep, sizeof *qp);
diff --git a/lib/flow.h b/lib/flow.h
index d8632ff..8ceb442 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -110,7 +110,13 @@  struct flow {
     struct eth_addr dl_dst;     /* Ethernet destination address. */
     struct eth_addr dl_src;     /* Ethernet source address. */
     ovs_be16 dl_type;           /* Ethernet frame type. */
-    ovs_be16 vlan_tci;          /* If 802.1Q, TCI | VLAN_CFI; otherwise 0. */
+    ovs_be16 vlan_tci;          /* If 802.1Q, TCI | VLAN_CFI; If 802.1ad,
+                                 * outer tag, otherwise 0. */
+    ovs_be16 vlan_ctci;         /* If 802.1ad, Customer TCI | VLAN_CFI,
+                                 * inner tag, otherwise 0. */
+    ovs_be16 vlan_tpid;         /* Vlan protocol type,
+                                 * either 802.1ad or 802.1q. */
+    uint32_t pad2;              /* Pad to 64 bits. */
     ovs_be32 mpls_lse[ROUND_UP(FLOW_MAX_MPLS_LABELS, 2)]; /* MPLS label stack
                                                              (with padding). */
     /* L3 (64-bit aligned) */
@@ -127,7 +133,7 @@  struct flow {
     struct eth_addr arp_sha;    /* ARP/ND source hardware address. */
     struct eth_addr arp_tha;    /* ARP/ND target hardware address. */
     ovs_be16 tcp_flags;         /* TCP flags. With L3 to avoid matching L4. */
-    ovs_be16 pad2;              /* Pad to 64 bits. */
+    ovs_be16 pad3;              /* Pad to 64 bits. */
 
     /* L4 (64-bit aligned) */
     ovs_be16 tp_src;            /* TCP/UDP/SCTP source port. */
@@ -153,7 +159,7 @@  BUILD_ASSERT_DECL(sizeof(struct flow_tnl) % sizeof(uint64_t) == 0);
 
 /* Remember to update FLOW_WC_SEQ when changing 'struct flow'. */
 BUILD_ASSERT_DECL(offsetof(struct flow, igmp_group_ip4) + sizeof(uint32_t)
-                  == sizeof(struct flow_tnl) + 192
+                  == sizeof(struct flow_tnl) + 200
                   && FLOW_WC_SEQ == 33);
 
 /* Incremental points at which flow classification may be performed in
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index c46ce97..ad88076 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1280,16 +1280,20 @@  format_STRIP_VLAN(const struct ofpact_null *a, struct ds *s)
 static enum ofperr
 decode_OFPAT_RAW11_PUSH_VLAN(ovs_be16 eth_type, struct ofpbuf *out)
 {
-    if (eth_type != htons(ETH_TYPE_VLAN_8021Q)) {
-        /* XXX 802.1AD(QinQ) isn't supported at the moment */
+    struct ofpact_push_vlan *oam;
+
+    if (!eth_type_vlan(eth_type)) {
+        /* XXX Only 802.1q or 802.1AD(QinQ) are supported.  */
         return OFPERR_OFPBAC_BAD_ARGUMENT;
     }
-    ofpact_put_PUSH_VLAN(out);
+    oam = ofpact_put_PUSH_VLAN(out);
+    oam->ethertype = eth_type;
+
     return 0;
 }
 
 static void
-encode_PUSH_VLAN(const struct ofpact_null *null OVS_UNUSED,
+encode_PUSH_VLAN(const struct ofpact_push_vlan *push_vlan,
                  enum ofp_version ofp_version, struct ofpbuf *out)
 {
     if (ofp_version == OFP10_VERSION) {
@@ -1297,7 +1301,7 @@  encode_PUSH_VLAN(const struct ofpact_null *null OVS_UNUSED,
          * follow this action. */
     } else {
         /* XXX ETH_TYPE_VLAN_8021AD case */
-        put_OFPAT11_PUSH_VLAN(out, htons(ETH_TYPE_VLAN_8021Q));
+        put_OFPAT11_PUSH_VLAN(out, push_vlan->ethertype);
     }
 }
 
@@ -1309,25 +1313,25 @@  parse_PUSH_VLAN(char *arg, struct ofpbuf *ofpacts,
     char *error;
 
     *usable_protocols &= OFPUTIL_P_OF11_UP;
-    error = str_to_u16(arg, "ethertype", &ethertype);
+    error = str_to_u16(arg, "push_vlan", &ethertype);
     if (error) {
         return error;
     }
 
-    if (ethertype != ETH_TYPE_VLAN_8021Q) {
-        /* XXX ETH_TYPE_VLAN_8021AD case isn't supported */
+    if (!eth_type_vlan(htons(ethertype))) {
+        /* Check for valid VLAN ethertypes */
         return xasprintf("%s: not a valid VLAN ethertype", arg);
     }
 
-    ofpact_put_PUSH_VLAN(ofpacts);
+    ofpact_put_PUSH_VLAN(ofpacts)->ethertype = htons(ethertype);
     return NULL;
 }
 
 static void
-format_PUSH_VLAN(const struct ofpact_null *a OVS_UNUSED, struct ds *s)
+format_PUSH_VLAN(const struct ofpact_push_vlan *a OVS_UNUSED, struct ds *s)
 {
     /* XXX 802.1AD case*/
-    ds_put_format(s, "push_vlan:%#"PRIx16, ETH_TYPE_VLAN_8021Q);
+    ds_put_format(s, "push_vlan:%#"PRIx16, ntohs(a->ethertype));
 }
 
 /* Action structure for OFPAT10_SET_DL_SRC/DST and OFPAT11_SET_DL_SRC/DST. */
@@ -5546,8 +5550,9 @@  ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
         return 0;
 
     case OFPACT_PUSH_VLAN:
-        if (flow->vlan_tci & htons(VLAN_CFI)) {
-            /* Multiple VLAN headers not supported. */
+        if (flow->vlan_tci & htons(VLAN_CFI) &&
+            flow->vlan_ctci & htons(VLAN_CFI)) {
+            /* More then 2 levels of VLAN headers not supported. */
             return OFPERR_OFPBAC_BAD_TAG;
         }
         /* Temporary mark that we have a vlan tag. */
@@ -6191,6 +6196,7 @@  ofpacts_get_meter(const struct ofpact ofpacts[], size_t ofpacts_len)
 
 /* Formatting ofpacts. */
 
+
 static void
 ofpact_format(const struct ofpact *a, struct ds *s)
 {
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 51b2963..57fa8e9 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -66,7 +66,7 @@ 
     OFPACT(SET_VLAN_VID,    ofpact_vlan_vid,    ofpact, "set_vlan_vid") \
     OFPACT(SET_VLAN_PCP,    ofpact_vlan_pcp,    ofpact, "set_vlan_pcp") \
     OFPACT(STRIP_VLAN,      ofpact_null,        ofpact, "strip_vlan")   \
-    OFPACT(PUSH_VLAN,       ofpact_null,        ofpact, "push_vlan")    \
+    OFPACT(PUSH_VLAN,       ofpact_push_vlan,   ofpact, "push_vlan")    \
     OFPACT(SET_ETH_SRC,     ofpact_mac,         ofpact, "mod_dl_src")   \
     OFPACT(SET_ETH_DST,     ofpact_mac,         ofpact, "mod_dl_dst")   \
     OFPACT(SET_IPV4_SRC,    ofpact_ipv4,        ofpact, "mod_nw_src")   \
@@ -401,7 +401,15 @@  struct ofpact_set_field {
     union mf_value mask;
 };
 
-/* OFPACT_PUSH_VLAN/MPLS/PBB
+/* OFPACT_PUSH_VLAN
+ *
+ * Used for OFPAT11_PUSH_VLAN. */
+struct ofpact_push_vlan {
+    struct ofpact ofpact;
+    ovs_be16 ethertype;
+};
+
+/* OFPACT_PUSH_MPLS
  *
  * Used for NXAST_PUSH_MPLS, OFPAT11_PUSH_MPLS. */
 struct ofpact_push_mpls {
@@ -411,7 +419,7 @@  struct ofpact_push_mpls {
 
 /* OFPACT_POP_MPLS
  *
- * Used for NXAST_POP_MPLS, OFPAT11_POP_MPLS.. */
+ * Used for NXAST_POP_MPLS, OFPAT11_POP_MPLS. */
 struct ofpact_pop_mpls {
     struct ofpact ofpact;
     ovs_be16 ethertype;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index de5c9b1..68bd6ce 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2795,6 +2795,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
     struct flow *flow = &ctx->xin->flow;
     struct flow_tnl flow_tnl;
     ovs_be16 flow_vlan_tci;
+    ovs_be16 flow_vlan_ctci;
     uint32_t flow_pkt_mark;
     uint8_t flow_nw_tos;
     odp_port_t out_port, odp_port;
@@ -2936,6 +2937,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
     }
 
     flow_vlan_tci = flow->vlan_tci;
+    flow_vlan_ctci = flow->vlan_ctci;
     flow_pkt_mark = flow->pkt_mark;
     flow_nw_tos = flow->nw_tos;
 
@@ -3062,6 +3064,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
  out:
     /* Restore flow */
     flow->vlan_tci = flow_vlan_tci;
+    flow->vlan_ctci = flow_vlan_ctci;
     flow->pkt_mark = flow_pkt_mark;
     flow->nw_tos = flow_nw_tos;
 }
@@ -4214,6 +4217,14 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             /* XXX 802.1AD(QinQ) */
             memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
             flow->vlan_tci = htons(VLAN_CFI);
+            if (ofpact_get_PUSH_VLAN(a)->ethertype == htons(ETH_TYPE_VLAN_8021AD)) {
+                memset(&wc->masks.vlan_ctci, 0xff, sizeof wc->masks.vlan_ctci);
+                flow->vlan_ctci |= htons(VLAN_CFI);
+                memset(&wc->masks.vlan_tpid, 0xff, sizeof wc->masks.vlan_tpid);
+                flow->vlan_tpid = htons(ETH_TYPE_VLAN_8021AD);
+            } else if (ofpact_get_PUSH_VLAN(a)->ethertype == htons(ETH_TYPE_VLAN_8021Q)) {
+                flow->vlan_tpid = htons(ETH_TYPE_VLAN_8021Q);
+            }
             break;
 
         case OFPACT_SET_ETH_SRC: