[ovs-dev] [PATCH] V3 Add Support for 802.1qad (qinq) Allows TPID of 0x88a8
Andy Zhou
azhou at nicira.com
Mon Apr 14 21:31:46 UTC 2014
It would be great if your next rebased patch also provide some
information on what has been tested.
Some higher level feedback follows, Minor feedback inline.
Patch format:
For ease of up-streaming to Linux kenrel, we should split the patch
into two parts. One
for kernel datapath, one for OVS user space.
Coding style:
Please note datapath code follows Linux kernel coding convention.
OVS user space follows OVS coding style (Documented in file CodingStyle)
User space:
ETH_TYPE_VLAN in packet.h is currently defined as ETH_TYPE_VLAN_8021Q.
You may want to grep for all use cases of ETH_TYPE_VLAN. Most cases may
need touch up. Some cases are harder to handle, for example in Normal actions.
Unless you have other ideas, I think we can let it slide for now -- It
will depend
on how we want to define 'normal action' for OVS.
Datapath:
flow_extract() also needs to handle 802.1AD.
I don't understand the logic of pop_vlan() .
To make the vlan handling logic consistent, OVS data patch internal
should always
put the outter vlan ethertyp in skb->protocol and skb->vlan_tci. For
kernel version 3.10 or later.
The networking core should handle this by testing whether a networking
driver properly support
vlan offloading. For earlier kernel version, we may have to always
flatten out 802.1ad vlan
right before hand an skb off to driver. Does this make sense?
push_vlan() may also need
touch up.
On Sun, Apr 13, 2014 at 4:12 PM, Tom Herbert <thomasfherbert at gmail.com> wrote:
> Signed-off-by: Tom Herbert <thomasfherbert at gmail.com>
> ---
> NEWS | 1 +
> datapath/actions.c | 16 ++++++++++++----
> datapath/flow_netlink.c | 15 +++++++++++++--
> include/linux/openvswitch.h | 12 ++++++------
> lib/odp-execute.c | 2 +-
> lib/odp-util.c | 2 +-
> lib/ofp-parse.c | 3 ++-
> lib/packets.c | 5 +++--
> 8 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 7925598..efd079c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -16,6 +16,7 @@ Post-v2.1.0
> - Support for Linux kernels up to 3.13. From Kernel 3.12 onwards 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..771a444 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -84,8 +84,9 @@ static int pop_vlan(struct sk_buff *skb)
> if (likely(vlan_tx_tag_present(skb))) {
> vlan_set_tci(skb, 0);
> } else {
> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN))
> + if (unlikely(((skb->protocol != htons(ETH_P_8021Q)) &&
> + (skb->protocol != htons(ETH_P_8021AD))) ||
> + (skb->len < VLAN_ETH_HLEN)))
> return 0;
>
> err = __pop_vlan_tci(skb, &tci);
> @@ -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));
> + 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);
> return 0;
> }
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 5c32cd0..22cfa65 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)) {
> + __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)) &&
> + (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
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 37e44e3..c9c101a 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -220,7 +220,7 @@ odp_execute_actions__(void *dp, struct ofpbuf *packet, bool steal,
>
> case OVS_ACTION_ATTR_PUSH_VLAN: {
> const struct ovs_action_push_vlan *vlan = nl_attr_get(a);
> - eth_push_vlan(packet, htons(ETH_TYPE_VLAN), vlan->vlan_tci);
> + eth_push_vlan(packet, vlan->vlan_tpid, vlan->vlan_tci);
> break;
> }
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index b58f1c0..dd32b09 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -432,7 +432,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
> case OVS_ACTION_ATTR_PUSH_VLAN:
> vlan = nl_attr_get(a);
> ds_put_cstr(ds, "push_vlan(");
> - if (vlan->vlan_tpid != htons(ETH_TYPE_VLAN)) {
> + if (vlan->vlan_tpid != htons(ETH_TYPE_VLAN_8021Q)) {
> ds_put_format(ds, "tpid=0x%04"PRIx16",", ntohs(vlan->vlan_tpid));
> }
> format_vlan_tci(ds, vlan->vlan_tci);
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index d250042..27bf0f0 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -704,7 +704,8 @@ parse_named_action(enum ofputil_action_code code,
> return error;
> }
>
> - if (ethertype != ETH_TYPE_VLAN_8021Q) {
> + if ((ethertype != ETH_TYPE_VLAN_8021Q) &&
> + (ethertype != ETH_TYPE_VLAN_8021AD)) {
> /* XXX ETH_TYPE_VLAN_8021AD case isn't supported */
Should we also fix this comment?
> return xasprintf("%s: not a valid VLAN ethertype", arg);
> }
> diff --git a/lib/packets.c b/lib/packets.c
> index 6244c3f..997bac6 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -198,8 +198,9 @@ eth_pop_vlan(struct ofpbuf *packet)
> {
> struct vlan_eth_header *veh = ofpbuf_l2(packet);
>
> - if (veh && ofpbuf_size(packet) >= sizeof *veh
> - && veh->veth_type == htons(ETH_TYPE_VLAN)) {
> + if (veh && ofpbuf_size(packet) >= sizeof *veh &&
> + ((veh->veth_type == htons(ETH_TYPE_VLAN_8021Q)) ||
> + ((veh->veth_type == htons(ETH_TYPE_VLAN_8021AD))))) {
>
> memmove((char *)veh + VLAN_HEADER_LEN, veh, 2 * ETH_ADDR_LEN);
> ofpbuf_resize_l2(packet, -VLAN_HEADER_LEN);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list