Hi Pieter,
On Thu, Aug 23, 2018 at 03:47:58PM +0100, Pieter Jansen van Vuuren wrote:
> Add TC offload support for classifying single MPLS tagged traffic.
>
> Signed-off-by: Pieter Jansen van Vuuren <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> Reviewed-by: John Hurley <[email protected]>
thanks for your patch. In general I am happy with this and would value any
feedback from others.
Travis noticed a few sparse problems which I have annotated inline.
> ---
> lib/netdev-tc-offloads.c | 14 ++++++--
> lib/tc.c | 69 ++++++++++++++++++++++++++++++++++++++++
> lib/tc.h | 1 +
> 3 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 7bc745e95..a12bc4c39 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -446,6 +446,10 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> match_set_dl_type(match, key->encap_eth_type[0]);
> }
> flow_fix_vlan_tpid(&match->flow);
> + } else if (eth_type_mpls(key->eth_type)) {
> + match->flow.mpls_lse[0] = key->mpls_lse & mask->mpls_lse;
> + match->wc.masks.mpls_lse[0] = mask->mpls_lse;
> + match_set_dl_type(match, key->encap_eth_type[0]);
> } else {
> match_set_dl_type(match, key->eth_type);
> }
> @@ -875,9 +879,9 @@ test_key_and_mask(struct match *match)
> return EOPNOTSUPP;
> }
>
> - for (int i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
> + for (int i = 1; i < FLOW_MAX_MPLS_LABELS; i++) {
> if (mask->mpls_lse[i]) {
> - VLOG_DBG_RL(&rl, "offloading attribute mpls_lse isn't
> supported");
> + VLOG_DBG_RL(&rl, "offloading multiple mpls_lses isn't
> supported");
> return EOPNOTSUPP;
> }
> }
> @@ -979,6 +983,12 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
>
> flower.key.eth_type = key->dl_type;
> flower.mask.eth_type = mask->dl_type;
> + if (mask->mpls_lse[0]) {
> + flower.key.mpls_lse = key->mpls_lse[0];
> + flower.mask.mpls_lse = mask->mpls_lse[0];
> + flower.key.encap_eth_type[0] = flower.key.eth_type;
> + }
> + mask->mpls_lse[0] = 0;
>
> if (mask->vlans[0].tci) {
> ovs_be16 vid_mask = mask->vlans[0].tci & htons(VLAN_VID_MASK);
> diff --git a/lib/tc.c b/lib/tc.c
> index bbc382326..2fbbaab17 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -272,6 +272,10 @@ static const struct nl_policy tca_flower_policy[] = {
> [TCA_FLOWER_KEY_SCTP_DST] = { .type = NL_A_U16, .optional = true, },
> [TCA_FLOWER_KEY_SCTP_SRC_MASK] = { .type = NL_A_U16, .optional = true, },
> [TCA_FLOWER_KEY_SCTP_DST_MASK] = { .type = NL_A_U16, .optional = true, },
> + [TCA_FLOWER_KEY_MPLS_TTL] = { .type = NL_A_U8, .optional = true, },
> + [TCA_FLOWER_KEY_MPLS_TC] = { .type = NL_A_U8, .optional = true, },
> + [TCA_FLOWER_KEY_MPLS_BOS] = { .type = NL_A_U8, .optional = true, },
> + [TCA_FLOWER_KEY_MPLS_LABEL] = { .type = NL_A_U32, .optional = true, },
> [TCA_FLOWER_KEY_VLAN_ID] = { .type = NL_A_U16, .optional = true, },
> [TCA_FLOWER_KEY_VLAN_PRIO] = { .type = NL_A_U8, .optional = true, },
> [TCA_FLOWER_KEY_VLAN_ETH_TYPE] = { .type = NL_A_U16, .optional = true, },
> @@ -344,6 +348,46 @@ nl_parse_flower_eth(struct nlattr **attrs, struct
> tc_flower *flower)
> }
> }
>
> +static void
> +nl_parse_flower_mpls(struct nlattr **attrs, struct tc_flower *flower)
> +{
> + uint8_t ttl, tc, bos;
> + ovs_be32 label;
> +
> + if (!eth_type_mpls(flower->key.eth_type)) {
> + return;
> + }
> +
> + flower->key.encap_eth_type[0] =
> + nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ETH_TYPE]);
> + flower->key.mpls_lse = 0;
> + flower->mask.mpls_lse = 0;
> +
> + if (attrs[TCA_FLOWER_KEY_MPLS_TTL]) {
> + ttl = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_MPLS_TTL]);
> + set_mpls_lse_ttl(&flower->key.mpls_lse, ttl);
> + set_mpls_lse_ttl(&flower->mask.mpls_lse, 0xff);
> + }
> +
> + if (attrs[TCA_FLOWER_KEY_MPLS_BOS]) {
> + bos = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_MPLS_BOS]);
> + set_mpls_lse_bos(&flower->key.mpls_lse, bos);
> + set_mpls_lse_ttl(&flower->mask.mpls_lse, 0xff);
> + }
> +
> + if (attrs[TCA_FLOWER_KEY_MPLS_TC]) {
> + tc = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_MPLS_TC]);
> + set_mpls_lse_tc(&flower->key.mpls_lse, tc);
> + set_mpls_lse_tc(&flower->mask.mpls_lse, 0xff);
> + }
> +
> + if (attrs[TCA_FLOWER_KEY_MPLS_LABEL]) {
> + label = nl_attr_get_be32(attrs[TCA_FLOWER_KEY_MPLS_LABEL]);
> + set_mpls_lse_label(&flower->key.mpls_lse, ntohl(label));
The line above seems to convert label from network to host order,
however, set_mpls_lse_label() expects a label to be big endian.
https://travis-ci.org/horms2/ovs/jobs/422509294
> + set_mpls_lse_label(&flower->mask.mpls_lse, OVS_BE32_MAX);
> + }
> +}
> +
> static void
> nl_parse_flower_vlan(struct nlattr **attrs, struct tc_flower *flower)
> {
> @@ -1035,6 +1079,7 @@ nl_parse_flower_options(struct nlattr *nl_options,
> struct tc_flower *flower)
> }
>
> nl_parse_flower_eth(attrs, flower);
> + nl_parse_flower_mpls(attrs, flower);
> nl_parse_flower_vlan(attrs, flower);
> nl_parse_flower_ip(attrs, flower);
> nl_parse_flower_tunnel(attrs, flower);
> @@ -1652,6 +1697,7 @@ nl_msg_put_flower_options(struct ofpbuf *request,
> struct tc_flower *flower)
> uint16_t host_eth_type = ntohs(flower->key.eth_type);
> bool is_vlan = eth_type_vlan(flower->key.eth_type);
> bool is_qinq = is_vlan && eth_type_vlan(flower->key.encap_eth_type[0]);
> + bool is_mpls = eth_type_mpls(flower->key.eth_type);
> int err;
>
> /* need to parse acts first as some acts require changing the matching
> @@ -1669,6 +1715,10 @@ nl_msg_put_flower_options(struct ofpbuf *request,
> struct tc_flower *flower)
> }
> }
>
> + if (is_mpls) {
> + host_eth_type = ntohs(flower->key.encap_eth_type[0]);
> + }
> +
> FLOWER_PUT_MASKED_VALUE(dst_mac, TCA_FLOWER_KEY_ETH_DST);
> FLOWER_PUT_MASKED_VALUE(src_mac, TCA_FLOWER_KEY_ETH_SRC);
>
> @@ -1711,6 +1761,25 @@ nl_msg_put_flower_options(struct ofpbuf *request,
> struct tc_flower *flower)
>
> nl_msg_put_be16(request, TCA_FLOWER_KEY_ETH_TYPE, flower->key.eth_type);
>
> + if (is_mpls) {
> + if (mpls_lse_to_ttl(flower->mask.mpls_lse)) {
> + nl_msg_put_u8(request, TCA_FLOWER_KEY_MPLS_TTL,
> + mpls_lse_to_ttl(flower->key.mpls_lse));
> + }
> + if (mpls_lse_to_tc(flower->mask.mpls_lse)) {
> + nl_msg_put_u8(request, TCA_FLOWER_KEY_MPLS_TC,
> + mpls_lse_to_tc(flower->key.mpls_lse));
> + }
> + if (mpls_lse_to_bos(flower->mask.mpls_lse)) {
> + nl_msg_put_u8(request, TCA_FLOWER_KEY_MPLS_BOS,
> + mpls_lse_to_bos(flower->key.mpls_lse));
> + }
> + if (mpls_lse_to_label(flower->mask.mpls_lse)) {
> + nl_msg_put_be32(request, TCA_FLOWER_KEY_MPLS_LABEL,
> + mpls_lse_to_label(flower->key.mpls_lse));
mpls_lse_to_label returns the label in host byte order but
a big endian value appears to be desired.
https://travis-ci.org/horms2/ovs/jobs/422509294
I wonder if a _be variant of mpls_lse_to_label() is a good solution to this
problem.
> + }
> + }
> +
> if (is_vlan) {
> if (flower->key.vlan_id[0] || flower->key.vlan_prio[0]) {
> nl_msg_put_u16(request, TCA_FLOWER_KEY_VLAN_ID,
> diff --git a/lib/tc.h b/lib/tc.h
> index aa8805df2..036aba245 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -78,6 +78,7 @@ struct tc_flower_key {
> struct eth_addr dst_mac;
> struct eth_addr src_mac;
>
> + ovs_be32 mpls_lse;
> ovs_be16 tcp_src;
> ovs_be16 tcp_dst;
> ovs_be16 tcp_flags;
> --
> 2.17.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
