On Wed, Sep 14, 2016 at 12:34:36PM +0800, Xiao Liang wrote:
> Flow key handleing changes:
>  - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
>    headers.
>  - Add dpif multi-VLAN capability probing. If datapath supports multi-VLAN,
>    increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
> 
> Refacter VLAN handling in dpif-xlate:
>  - Introduce 'xvlan' to track VLAN stack during flow processing.
>  - Input and output VLAN translation according to the xbundle type.
> 
> Push VLAN action support:
>  - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
>  - Support push_vlan on dot1q packets.
> 
> Add new port VLAN mode "dot1q-tunnel":
>  - Example:
>      ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100
>    Pushes another VLAN 100 header on packets (tagged and untagged) on ingress,
>    and pops it on egress.
>  - Customer VLAN check:
>      ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 cvlans=10,20
>    Only customer VLAN of 10 and 20 are allowed.
> 
> Use other_config:vlan-limit in table Open_vSwitch to limit maxium VLANs
> that can be matched
> 
> Add test cases for VLAN depth limit, Multi-VLAN actions and QinQ VLAN handling
> 
> Signed-off-by: Xiao Liang <shaw.l...@gmail.com>

Thanks for working on this.

"sparse" says:
    ../lib/odp-util.c:5052:42: warning: restricted ovs_be16 degrades to integer

The fix is:

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 527cd7a..12ebda1 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -5049,7 +5049,7 @@ parse_8021q_onward(const struct nlattr 
*attrs[OVS_KEY_ATTR_MAX + 1],
 
     while (encaps < max_vlan_headers &&
            (is_mask ?
-            (src_flow->vlans[encaps].tci & htons(VLAN_CFI)) :
+            (src_flow->vlans[encaps].tci & htons(VLAN_CFI)) != 0 :
             eth_type_vlan(flow->dl_type))) {
         /* Calculate fitness of outer attributes. */
         encap  = (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)

In miniflow_extract(), the following code always adds an extra 64-bit
word to the miniflow, even if there are no vlans.  It should avoid that
if the vlan doubleword is going to be all-0s.  (Previously, this
optimization was not necessary because the VLAN was in the same
doubleword as the Ethertype, which is always nonzero.)

        /* dl_type */
        dl_type = parse_ethertype(&data, &size);
        miniflow_push_be16(mf, dl_type, dl_type);
        miniflow_pad_to_64(mf, dl_type);
        miniflow_push_words_32(mf, vlans, vlans, FLOW_MAX_VLAN_HEADERS);

In flow_wildcards_init_for_packet(), I think that the safer thing to do
would be to implement the FIXME:
+    /* No need to set mask of inner VLANs that doesn't exist.
+     * FIXME: set mask of the first zero VLAN? */
+    WC_MASK_FIELD(wc, vlans[0]);
+    for (int i = 1; i < FLOW_MAX_VLAN_HEADERS; i++) {
+        if (flow->vlans[i].tci == htons(0)) {
+            break;
+        }
+        WC_MASK_FIELD(wc, vlans[i]);
+    }

I don't think that a max_vlan_headers global variable is a good idea.
Different datapaths might have different limits.

I'm still really skeptical of the value of the whole "xvlan" data
structure in ofproto-dpif-xlate.  It seems like it adds an extra stage
of translation to and from a new format, but I don't see how it adds
much value.  Can you show me where it removes a significant amount of
code, or makes code much clearer, or has other value?

Thanks,

Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to