[ovs-dev] Subject: [PATCH] Add support for 802.1ad (qinq)
Andy Zhou
azhou at nicira.com
Wed Apr 2 19:54:47 UTC 2014
Thomas, thanks a lot for working on this.
The patch seems to be incomplete. Is this intentional? If yes, then we
may want delay mentioning it
in the news.
1) User space open flow PUSH_VLAN currently blocking 802.1ad header.
That needs to be fixed for
open flow versions that supports it.
2) User space VLAN flow handling may need adjustments as well
3) Kernel data receiving path needs to accept 802.1ad header.
On Wed, Apr 2, 2014 at 9:07 AM, Thomas F Herbert
<thomasfherbert at gmail.com> wrote:
> From c7521a74f0e8f2673d4f542b885f6c82a0302f45 Mon Sep 17 00:00:00 2001
> From: Tom Herbert <thomasfherbert at gmail.com>
> Date: Wed, 2 Apr 2014 06:03:23 +0000
> Add support for 802.1ad (qinq)
> Allows 0x88a8 for TPID is S-VLAN (outer) tag.
> Signed-off-by: Tom Herbert <thomasfherbert at gmail.com>
>
> ---
> NEWS | 1 +
> datapath/actions.c | 12 ++++++++++--
> datapath/flow_netlink.c | 15 +++++++++++++--
> include/linux/openvswitch.h | 12 ++++++------
> 4 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 4f88e3b..201b0b6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -13,6 +13,7 @@ Post-v2.1.0
> - Support for Linux kernels up to 3.12. On Kernel 3.12 OVS uses tunnel
> API for GRE and VXLAN.
> - Added DPDK support.
> + - Added support for 802.1qad
>
>
> v2.1.0 - xx xxx xxxx
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 0b66e7c..91375d1 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -85,6 +85,7 @@ static int pop_vlan(struct sk_buff *skb)
> vlan_set_tci(skb, 0);
> } else {
> if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
Should this be && ?
> + skb->protocol != htons(ETH_P_8021AD) ||
> skb->len < VLAN_ETH_HLEN))
> return 0;
>
> @@ -101,7 +102,11 @@ static int pop_vlan(struct sk_buff *skb)
> if (unlikely(err))
> return err;
>
> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
> + if (unlikely(skb->protocol == htons(ETH_P_8021AD)))
> + __vlan_put_tag(skb, htons(ETH_P_8021AD), ntohs(tci));
It looks odd that they use slightly different APIs.
> + else
> + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
> +
> return 0;
> }
>
> @@ -121,7 +126,10 @@ static int push_vlan(struct sk_buff *skb, const struct
> ovs_action_push_vlan *vla
> + (2 * ETH_ALEN), VLAN_HLEN, 0));
>
> }
> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) &
> ~VLAN_TAG_PRESENT);
> + if (likely(vlan->vlan_tpid == htons(ETH_P_8021Q)))
> + __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci)
> & ~VLAN_TAG_PRESENT);
> + else
> + __vlan_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) &
> ~VLAN_TAG_PRESENT);
Same API comment here.
> return 0;
> }
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 5c32cd0..855af32 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -748,7 +748,8 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>
> if ((key_attrs & (1ULL << OVS_KEY_ATTR_ETHERNET)) &&
> (key_attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) &&
> - (nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021Q))) {
> + ((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021Q)) ||
> + (nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021AD))))
> {
> __be16 tci;
>
> if (!((key_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) &&
> @@ -922,6 +923,15 @@ int ovs_nla_put_flow(const struct sw_flow_key *swkey,
> encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
> if (!swkey->eth.tci)
> goto unencap;
> + } else if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021AD)) {
I am not sure if we need a separate else here, Why not just combine
with ETH_P_8021Q above?
> + __be16 eth_type;
> + eth_type = !is_mask ? htons(ETH_P_8021AD) : htons(0xffff);
> + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
> + nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci))
> + goto nla_put_failure;
> + encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
> + if (!swkey->eth.tci)
> + goto unencap;
> } else
> encap = NULL;
>
> @@ -1455,7 +1465,8 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
>
> case OVS_ACTION_ATTR_PUSH_VLAN:
> vlan = nla_data(a);
> - if (vlan->vlan_tpid != htons(ETH_P_8021Q))
> + if ((vlan->vlan_tpid != htons(ETH_P_8021Q)) ||
Should this be && ?
> + (vlan->vlan_tpid != htons(ETH_P_8021AD)))
> return -EINVAL;
> if (!(vlan->vlan_tci & htons(VLAN_TAG_PRESENT)))
> return -EINVAL;
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index a88f6f1..5a82970 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -525,14 +525,14 @@ struct ovs_action_push_mpls {
> * @vlan_tci: Tag control identifier (TCI) to push. The CFI bit must be
> set
> * (but it will not be set in the 802.1Q header that is pushed).
> *
> - * The @vlan_tpid value is typically %ETH_P_8021Q. The only acceptable
> TPID
> - * values are those that the kernel module also parses as 802.1Q headers,
> to
> + * The @vlan_tpid value is typically %ETH_P_8021Q or %ETH_P_8021AD. The
> only acceptable
> + * TPID values are those that the kernel module also parses as 802.1Q
> headers, to
> * prevent %OVS_ACTION_ATTR_PUSH_VLAN followed by %OVS_ACTION_ATTR_POP_VLAN
> * from having surprising results.
> */
> struct ovs_action_push_vlan {
> - __be16 vlan_tpid; /* 802.1Q TPID. */
> - __be16 vlan_tci; /* 802.1Q TCI (VLAN ID and priority). */
> + __be16 vlan_tpid; /* 802.1Q or 802.1ad TPID. */
> + __be16 vlan_tci; /* 802.1Q or 802.1ad TCI (VLAN ID and priority). */
> };
>
> /* Data path hash algorithm for computing Datapath hash.
> @@ -565,9 +565,9 @@ struct ovs_action_recirc {
> * @OVS_ACTION_ATTR_OUTPUT: Output packet to port.
> * @OVS_ACTION_ATTR_USERSPACE: Send packet to userspace according to nested
> * %OVS_USERSPACE_ATTR_* attributes.
> - * @OVS_ACTION_ATTR_PUSH_VLAN: Push a new outermost 802.1Q header onto the
> + * @OVS_ACTION_ATTR_PUSH_VLAN: Push a new outermost 802.1Q or 802.1ad
> header onto the
> * packet.
> - * @OVS_ACTION_ATTR_POP_VLAN: Pop the outermost 802.1Q header off the
> packet.
> + * @OVS_ACTION_ATTR_POP_VLAN: Pop the outermost 802.1Q or 802.1ad header
> off the packet.
> * @OVS_ACTION_ATTR_SAMPLE: Probabilitically executes actions, as specified
> in
> * the nested %OVS_SAMPLE_ATTR_* attributes.
> * @OVS_ACTION_ATTR_SET: Replaces the contents of an existing header. The
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list