On Wed, Mar 1, 2017 at 2:47 PM, Eric Garver <[email protected]> wrote:
> Flow key handling 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.
>
> Refactor 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.
>
> Use other_config:vlan-limit in table Open_vSwitch to limit maximum VLANs
> that can be matched. This allows us to preserve backwards compatibility.
>
> Add test cases for VLAN depth limit, Multi-VLAN actions and QinQ VLAN
> handling
>
> Co-authored-by: Thomas F Herbert <[email protected]>
> Signed-off-by: Thomas F Herbert <[email protected]>
> Co-authored-by: Xiao Liang <[email protected]>
> Signed-off-by: Xiao Liang <[email protected]>
> Signed-off-by: Eric Garver <[email protected]>
Thanks for working on this. And sorry it took a while to get those reviewed.
The patch does not apply cleanly to master. Do you mind rebase and repost?
I suppose you will need to this anyways for them to be pushed.
I have a few comments for this patch, I will review this other
patches more carefully
after rebase.
git am complained about extra while space. I think it is about the
line below ofproto_set_vlan_limit()
>
> v2.7.0 - xx xxx xxxx
> ---------------------
> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> index df80dfe46199..0ed02e08fd2d 100644
> --- a/include/openvswitch/flow.h
> +++ b/include/openvswitch/flow.h
> @@ -23,7 +23,7 @@
> /* This sequence number should be incremented whenever anything involving
> flows
> * or the wildcarding of flows changes. This will cause build assertion
> * failures in places which likely need to be updated. */
> -#define FLOW_WC_SEQ 36
> +#define FLOW_WC_SEQ 37
>
> /* Number of Open vSwitch extension 32-bit registers. */
> #define FLOW_N_REGS 16
> @@ -61,6 +61,12 @@ const char *flow_tun_flag_to_string(uint32_t flags);
> /* Maximum number of supported MPLS labels. */
> #define FLOW_MAX_MPLS_LABELS 3
>
> +/* Maximum number of supported VLAN headers. */
> +#define FLOW_MAX_VLAN_HEADERS 2
> +
> +/* Legacy maximum VLAN headers */
> +#define LEGACY_MAX_VLAN_HEADERS 1
> +
> /*
> * A flow in the network.
> *
> @@ -103,7 +109,8 @@ 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. */
> + uint8_t pad2[2]; /* Pad to 64 bits. */
Do we need 4 more bytes for 64 bit alignment?
> +void
> +flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
> +{
> + int n = flow_count_vlan_headers(flow);
> + if (n == 0) {
> + return;
> + }
Can n be 0? or should this be an assert instead?
> + if (wc) {
> + memset(&wc->masks.vlans[1], 0xff,
> + sizeof(union flow_vlan_hdr) * (n - 1));
> + }
Should we also set the mask for wc->masks.vlans[0]?
> + memmove(&flow->vlans[0], &flow->vlans[1],
> + sizeof(union flow_vlan_hdr) * (n - 1));
> + memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> +}
> +
> +void
> +flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
> +{
> + int n = flow_count_vlan_headers(flow);
Do we always know there is room in the flow?
> + if (wc) {
> + memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> + }
Should we set mask for the vlan fields that is about to be pushed?
> + memmove(&flow->vlans[1], &flow->vlans[0],
> + sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
> + memset(&flow->vlans[0], 0, sizeof(union flow_vlan_hdr));
The function syas "_uninit", I don't think the last memset() is needed.
> }
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
