On Tuesday, December 30, 2014 10:12:59 AM you wrote:
> This is the linux kernel datapath portion of the 802.1AD patch.
>
>
> Signed-off-by: Thomas F Herbert <thomasfherb...@entpnt.com>
>
> ---
> datapath/actions.c | 32 ++++++---
> datapath/flow.c | 80
> +++++++++++++++++++----
> datapath/flow.h | 1 +
> datapath/flow_netlink.c | 62 ++++++++++++++++--
> datapath/linux/compat/include/linux/openvswitch.h | 16 ++---
> 5 files changed, 153 insertions(+), 38 deletions(-)
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 5a1dbe2..baa82e6 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -250,17 +250,18 @@ static int __pop_vlan_tci(struct sk_buff *skb, __be16
> *current_tci)
>
> return 0;
> }
> -
> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
> {
> __be16 tci;
> int err;
> + __be16 prot;
>
> 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);
> @@ -269,9 +270,9 @@ static int pop_vlan(struct sk_buff *skb, struct
> sw_flow_key *key)
> }
> /* move next vlan tag to hw accel tag */
> if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN)) {
> - key->eth.tci = 0;
> - return 0;
> + skb->len < VLAN_ETH_HLEN)) {
> + key->eth.tci = 0;
> + return 0;
> }
Please, don't mix up with code cleanups.
> invalidate_flow_key(key);
> @@ -279,7 +280,13 @@ static int pop_vlan(struct sk_buff *skb, struct
> sw_flow_key *key)
> if (unlikely(err))
> return err;
>
> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
> + if (unlikely(skb->protocol == htons(ETH_P_8021AD)))
> + prot = htons(ETH_P_8021AD);
> + else
> + prot = htons(ETH_P_8021Q);
> +
> + __vlan_hwaccel_put_tag(skb, prot, ntohs(tci));
> +
Why not simply:
__vlan_hwaccel_put_tag(skb, skb->protocol, ntohs(tci));
> return 0;
> }
>
> @@ -297,15 +304,20 @@ static int push_vlan(struct sk_buff *skb, struct
> sw_flow_key *key,
> /* Update mac_len for subsequent MPLS actions */
> skb->mac_len += VLAN_HLEN;
>
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
> + (2 * ETH_ALEN), VLAN_HLEN, 0));
Could you elaborate on the above change? I don't see why
the checksum is needed in all cases.
> invalidate_flow_key(key);
> } else {
> key->eth.tci = vlan->vlan_tci;
> }
> - __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);
> + skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> + }
> return 0;
break long lines?
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 69b13b3..d549fae 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -296,21 +296,75 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
> static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
> {
> struct qtag_prefix {
> - __be16 eth_type; /* ETH_P_8021Q */
> + __be16 eth_type; /* ETH_P_8021Q or ETH_P_8021AD */
> __be16 tci;
> };
> - struct qtag_prefix *qp;
> + struct qtag_prefix *qp = (struct qtag_prefix *) skb->data;
>
> - if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
> - return 0;
> + struct qinqtag_prefix {
> + __be16 eth_type; /* ETH_P_8021Q or ETH_P_8021AD */
> + __be16 tci;
> + __be16 encapsulated_proto; /* ETH_P_8021Q */
> + __be16 ctci;
> + };
>
> - if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
> - sizeof(__be16))))
> - return -ENOMEM;
> + if (likely(skb->vlan_tci | htons(VLAN_TAG_PRESENT))) {
> +
> + key->eth.tci = skb->vlan_tci | htons(VLAN_TAG_PRESENT);
> +
> + if(unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
^ missing space
> + struct qtag_prefix *qp = (struct qtag_prefix *)
> skb->data;
again?
> +
> + if (unlikely(skb->len < sizeof(struct qtag_prefix) +
> + sizeof(__be16)))
> + return 0;
This check is included in the pskb_may_pull() below.
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct
> qtag_prefix) +
> + sizeof(__be16)))) {
> + return -ENOMEM;
> + }
> +
I'd suggest to use offsetof(struct qtag_prefix, ctci), but I am not really
sure if it's portable. Or simply define the ctci_offset above as
sizeof(struct qtag_prefix) + sizeof(__be16) for better readability.
> + if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
> + key->eth.ctci = qp->tci |
> htons(VLAN_TAG_PRESENT);
> + __skb_pull(skb, sizeof(struct qtag_prefix));
> + }
> + }
> + return 0;
> + }
> +
> +
> + if (qp->eth_type == htons(ETH_P_8021AD)) {
> + struct qinqtag_prefix *qinqp = (struct qinqtag_prefix *)
> skb->data;
>
> - qp = (struct qtag_prefix *) skb->data;
> - key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
> - __skb_pull(skb, sizeof(struct qtag_prefix));
> + if (unlikely(skb->len < sizeof(struct qinqtag_prefix) +
> + sizeof(__be16)))
> + return 0;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct qinqtag_prefix) +
> + sizeof(__be16)))) {
> + return -ENOMEM;
> + }
Same above
> + key->eth.tci = qinqp->tci | htons(VLAN_TAG_PRESENT);
> + key->eth.ctci = qinqp->ctci | htons(VLAN_TAG_PRESENT);
> +
> + __skb_pull(skb, sizeof(struct qinqtag_prefix));
> +
> + return 0;
> +
> + }
> + if (qp->eth_type == htons(ETH_P_8021Q)) {
> +
> + if (unlikely(skb->len < sizeof(struct qtag_prefix) +
> + sizeof(__be16)))
> + return -ENOMEM;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
> + sizeof(__be16))))
> + return 0;
> + key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
> +
> + __skb_pull(skb, sizeof(struct qtag_prefix));
> + }
>
> return 0;
> }
> @@ -472,9 +526,9 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> */
>
> key->eth.tci = 0;
> - if (vlan_tx_tag_present(skb))
> - key->eth.tci = htons(vlan_get_tci(skb));
> - else if (eth->h_proto == htons(ETH_P_8021Q))
> + if ((vlan_tx_tag_present(skb)) ||
> + (eth->h_proto == htons(ETH_P_8021Q)) ||
> + (eth->h_proto == htons(ETH_P_8021AD)))
> if (unlikely(parse_vlan(skb, key)))
> return -ENOMEM;
>
> diff --git a/datapath/flow.h b/datapath/flow.h
> index 2bbf789..0bf0477 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -134,6 +134,7 @@ struct sw_flow_key {
> u8 src[ETH_ALEN]; /* Ethernet source address. */
> u8 dst[ETH_ALEN]; /* Ethernet destination address. */
> __be16 tci; /* 0 if no VLAN, VLAN_TAG_PRESENT set
> otherwise. */
> + __be16 ctci; /* 0 if no VLAN, VLAN_TAG_PRESENT set
> otherwise. */
> __be16 type; /* Ethernet frame type. */
> } eth;
> union {
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index c611e71..2e307ba 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -274,7 +274,7 @@ size_t ovs_key_attr_size(void)
>
> return nla_total_size(4) /* OVS_KEY_ATTR_PRIORITY */
> + nla_total_size(0) /* OVS_KEY_ATTR_TUNNEL */
> - + ovs_tun_key_attr_size()
> + + ovs_tun_key_attr_size()
please, no code cleanups
> + nla_total_size(4) /* OVS_KEY_ATTR_IN_PORT */
> + nla_total_size(4) /* OVS_KEY_ATTR_SKB_MARK */
> + nla_total_size(4) /* OVS_KEY_ATTR_DP_HASH */
> @@ -681,6 +681,28 @@ static int metadata_from_nlattrs(struct sw_flow_match
> *match, u64 *attrs,
> }
> return 0;
> }
> +static int ovs_nested_vlan_from_nlattrs(struct sw_flow_match *match,
> + u64 attrs, const struct nlattr **a,
> + bool is_mask, bool log)
> +{
> + if (attrs & (1ULL << OVS_KEY_ATTR_VLAN)) { /* This should be nested
> inner or "customer" tci" */
line is too long, perhaps put the comment above the if() ?
> + __be16 ctci;
> +
> + ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
> + if (!(ctci & htons(VLAN_TAG_PRESENT))) {
> + if (is_mask)
> + OVS_NLERR(log, "VLAN TCI mask does not have
> exact match for VLAN_TAG_PRESENT bit.");
> + else
> + OVS_NLERR(log, "VLAN TCI does not have
> VLAN_TAG_PRESENT bit set.");
> +
> + return -EINVAL;
> + }
> +
> + SW_FLOW_KEY_PUT(match, eth.ctci, ctci, is_mask);
> + attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
> + }
> + return 0;
> +}
>
> static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
> const struct nlattr **a, bool is_mask,
> @@ -720,6 +742,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
> *match, u64 attrs,
> attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
> }
>
> +
extra line...
> if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
> __be16 eth_type;
>
> @@ -969,8 +992,9 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> return err;
>
> 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))) {
> + (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_8021AD)))) {
> __be16 tci;
The indentation here is odd.
> if (!((key_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) &&
> @@ -986,9 +1010,23 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> encap_valid = true;
>
> if (tci & htons(VLAN_TAG_PRESENT)) {
> - err = parse_flow_nlattrs(encap, a, &key_attrs, log);
> - if (err)
> - return err;
> + u64 v_attrs = 0;
> +
> + if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE])
> == htons(ETH_P_8021AD)))) {
> +
> + err = parse_flow_nlattrs(encap, a, &v_attrs,
> log);
break long lines?
> + if (err)
> + return err;
> + if (v_attrs) {
> + err =
> ovs_nested_vlan_from_nlattrs(match, v_attrs, a, false, log);
> + if (err)
> + return err;
> + }
> + } else {
> + err = parse_flow_nlattrs(encap, a, &key_attrs,
> log);
> + if (err)
> + return err;
> + }
> } else if (!tci) {
> /* Corner case for truncated 802.1Q header. */
> if (nla_len(encap)) {
> @@ -1191,6 +1229,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;
>
> @@ -1828,7 +1875,8 @@ static 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/datapath/linux/compat/include/linux/openvswitch.h
> b/datapath/linux/compat/include/linux/openvswitch.h
> index 67715f8..6b5538a 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -570,14 +570,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
> - * prevent %OVS_ACTION_ATTR_PUSH_VLAN followed by %OVS_ACTION_ATTR_POP_VLAN
> - * from having surprising results.
> + * 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.
> @@ -629,9 +629,9 @@ struct ovs_action_push_tnl {
> * @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.
Break long lines.
> * @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
>
Thanks
fbl
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
