On 30/08/2018 14:28, Simon Horman wrote:
> 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.
Thanks Simon, I'll fix this up and repost.
>
>> + }
>> + }
>> +
>> 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
