diff mbox series

[ovs-dev] netdev-offload-tc: Revert tunnel src/dst port masks handling

Message ID 20200616130357.23719-1-roid@mellanox.com
State New
Headers show
Series [ovs-dev] netdev-offload-tc: Revert tunnel src/dst port masks handling | expand

Commit Message

Roi Dayan June 16, 2020, 1:03 p.m. UTC
The cited commit intended to add tc support for masking tunnel src/dst
ips and ports. It's not possible to do tunnel ports masking with
openflow rules and the default mask for tunnel ports set to 0 in
tnl_wc_init(), unlike tunnel ports default mask which is full mask.
So instead of never passing tunnel ports to tc, revert the changes
to tunnel ports to always pass the tunnel port.
In sw classification is done by the kernel, but for hw we must match
the tunnel dst port.

Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
Signed-off-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
---
 NEWS                        |  2 --
 include/openvswitch/match.h |  3 ---
 lib/match.c                 | 13 -------------
 lib/netdev-offload-tc.c     | 13 ++-----------
 lib/tc.c                    | 28 ++--------------------------
 tests/tunnel.at             |  4 ++--
 6 files changed, 6 insertions(+), 57 deletions(-)

Comments

Roi Dayan June 16, 2020, 2:06 p.m. UTC | #1
On 2020-06-16 4:03 PM, Roi Dayan wrote:
> The cited commit intended to add tc support for masking tunnel src/dst
> ips and ports. It's not possible to do tunnel ports masking with
> openflow rules and the default mask for tunnel ports set to 0 in
> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
> So instead of never passing tunnel ports to tc, revert the changes
> to tunnel ports to always pass the tunnel port.
> In sw classification is done by the kernel, but for hw we must match
> the tunnel dst port.
> 
> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Eli Britstein <elibr@mellanox.com>
> ---
>  NEWS                        |  2 --
>  include/openvswitch/match.h |  3 ---
>  lib/match.c                 | 13 -------------
>  lib/netdev-offload-tc.c     | 13 ++-----------
>  lib/tc.c                    | 28 ++--------------------------
>  tests/tunnel.at             |  4 ++--
>  6 files changed, 6 insertions(+), 57 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 88b273a0af89..22cacda20ac7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -19,8 +19,6 @@ Post-v2.13.0
>     - Tunnels: TC Flower offload
>       * Tunnel Local endpoint address masked match are supported.
>       * Tunnel Romte endpoint address masked match are supported.
> -     * Tunnel Local endpoint ports masked match are supported.
> -     * Tunnel Romte endpoint ports masked match are supported.
>  
>  
>  v2.13.0 - 14 Feb 2020
> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> index 9e480318e2b3..2e8812048e86 100644
> --- a/include/openvswitch/match.h
> +++ b/include/openvswitch/match.h
> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
> -void match_set_tun_tp_src_masked(struct match *match,
> -                                 ovs_be16 port, ovs_be16 mask);
>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
> diff --git a/lib/match.c b/lib/match.c
> index a77554851146..ba716579d8ec 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>  }
>  
>  void
> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
> -{
> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
> -}
> -
> -void
> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> -{
> -    match->wc.masks.tunnel.tp_src = mask;
> -    match->flow.tunnel.tp_src = port & mask;
> -}
> -
> -void
>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
>  {
>      match->wc.masks.tunnel.gbp_id = mask;
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index aa6d22e74f29..258d31f54b08 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
>                                       flower->mask.tunnel.ttl);
>          }
> -        if (flower->mask.tunnel.tp_dst) {
> -            match_set_tun_tp_dst_masked(match,
> -                                        flower->key.tunnel.tp_dst,
> -                                        flower->mask.tunnel.tp_dst);
> -        }
> -        if (flower->mask.tunnel.tp_src) {
> -            match_set_tun_tp_src_masked(match,
> -                                        flower->key.tunnel.tp_src,
> -                                        flower->mask.tunnel.tp_src);
> +        if (flower->key.tunnel.tp_dst) {
> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
>          }
>          if (flower->key.tunnel.metadata.present.len) {
>              flower_tun_opt_to_match(match, flower);
> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
>          flower.tunnel = true;
> diff --git a/lib/tc.c b/lib/tc.c
> index 29b4328d8bfc..c2ab77553306 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
>                                             .optional = true, },
>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
>                                            .optional = true, },
> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
> -                                          .optional = true, },
> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
> -                                               .optional = true, },
> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
> -                                               .optional = true, },
>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>          flower->key.tunnel.ipv6.ipv6_dst =
>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
>      }
> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
> -        flower->mask.tunnel.tp_src =
> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
> -        flower->key.tunnel.tp_src =
> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
> -    }
> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
> -        flower->mask.tunnel.tp_dst =
> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
>          flower->key.tunnel.tp_dst =
>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>      }
> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
>      uint8_t tos = flower->key.tunnel.tos;
>      uint8_t ttl = flower->key.tunnel.ttl;
> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
>      }
> -    if (tp_dst_mask) {
> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
> -                        tp_dst_mask);
> +    if (tp_dst) {
>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
>      }
> -    if (tp_src_mask) {
> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
> -                        tp_src_mask);
> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
> -    }
>      if (id_mask) {
>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>      }
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index a74a67aa8123..e08fd1e048f8 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
>      p2 2/2: (dummy)
>  ])
>  
> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>  
>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>  ])
>  
>  OVS_VSWITCHD_STOP
> 


travis ci results link
https://travis-ci.org/github/roidayan/ovs/builds/698910052
Roi Dayan June 16, 2020, 3:29 p.m. UTC | #2
On 2020-06-16 5:06 PM, Roi Dayan wrote:
> 
> 
> On 2020-06-16 4:03 PM, Roi Dayan wrote:
>> The cited commit intended to add tc support for masking tunnel src/dst
>> ips and ports. It's not possible to do tunnel ports masking with
>> openflow rules and the default mask for tunnel ports set to 0 in
>> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
>> So instead of never passing tunnel ports to tc, revert the changes
>> to tunnel ports to always pass the tunnel port.
>> In sw classification is done by the kernel, but for hw we must match
>> the tunnel dst port.
>>
>> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>> Reviewed-by: Eli Britstein <elibr@mellanox.com>
>> ---
>>  NEWS                        |  2 --
>>  include/openvswitch/match.h |  3 ---
>>  lib/match.c                 | 13 -------------
>>  lib/netdev-offload-tc.c     | 13 ++-----------
>>  lib/tc.c                    | 28 ++--------------------------
>>  tests/tunnel.at             |  4 ++--
>>  6 files changed, 6 insertions(+), 57 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 88b273a0af89..22cacda20ac7 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -19,8 +19,6 @@ Post-v2.13.0
>>     - Tunnels: TC Flower offload
>>       * Tunnel Local endpoint address masked match are supported.
>>       * Tunnel Romte endpoint address masked match are supported.
>> -     * Tunnel Local endpoint ports masked match are supported.
>> -     * Tunnel Romte endpoint ports masked match are supported.
>>  
>>  
>>  v2.13.0 - 14 Feb 2020
>> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
>> index 9e480318e2b3..2e8812048e86 100644
>> --- a/include/openvswitch/match.h
>> +++ b/include/openvswitch/match.h
>> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
>>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
>>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
>>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
>> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
>> -void match_set_tun_tp_src_masked(struct match *match,
>> -                                 ovs_be16 port, ovs_be16 mask);
>>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
>>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
>>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
>> diff --git a/lib/match.c b/lib/match.c
>> index a77554851146..ba716579d8ec 100644
>> --- a/lib/match.c
>> +++ b/lib/match.c
>> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>>  }
>>  
>>  void
>> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
>> -{
>> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
>> -}
>> -
>> -void
>> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>> -{
>> -    match->wc.masks.tunnel.tp_src = mask;
>> -    match->flow.tunnel.tp_src = port & mask;
>> -}
>> -
>> -void
>>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
>>  {
>>      match->wc.masks.tunnel.gbp_id = mask;
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index aa6d22e74f29..258d31f54b08 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
>>                                       flower->mask.tunnel.ttl);
>>          }
>> -        if (flower->mask.tunnel.tp_dst) {
>> -            match_set_tun_tp_dst_masked(match,
>> -                                        flower->key.tunnel.tp_dst,
>> -                                        flower->mask.tunnel.tp_dst);
>> -        }
>> -        if (flower->mask.tunnel.tp_src) {
>> -            match_set_tun_tp_src_masked(match,
>> -                                        flower->key.tunnel.tp_src,
>> -                                        flower->mask.tunnel.tp_src);
>> +        if (flower->key.tunnel.tp_dst) {
>> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
>>          }
>>          if (flower->key.tunnel.metadata.present.len) {
>>              flower_tun_opt_to_match(match, flower);
>> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
>>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
>>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
>> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
>> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
>>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
>>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
>>          flower.tunnel = true;
>> diff --git a/lib/tc.c b/lib/tc.c
>> index 29b4328d8bfc..c2ab77553306 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
>>                                             .optional = true, },
>>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
>>                                            .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
>> -                                          .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
>> -                                               .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
>> -                                               .optional = true, },
>>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
>>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
>>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
>> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>>          flower->key.tunnel.ipv6.ipv6_dst =
>>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
>>      }
>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
>> -        flower->mask.tunnel.tp_src =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
>> -        flower->key.tunnel.tp_src =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
>> -    }
>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
>> -        flower->mask.tunnel.tp_dst =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
>> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
>>          flower->key.tunnel.tp_dst =
>>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>>      }
>> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
>>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
>>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
>> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
>> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
>>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
>> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
>>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
>>      uint8_t tos = flower->key.tunnel.tos;
>>      uint8_t ttl = flower->key.tunnel.ttl;
>> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
>>      }
>> -    if (tp_dst_mask) {
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
>> -                        tp_dst_mask);
>> +    if (tp_dst) {
>>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
>>      }
>> -    if (tp_src_mask) {
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
>> -                        tp_src_mask);
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
>> -    }
>>      if (id_mask) {
>>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>>      }
>> diff --git a/tests/tunnel.at b/tests/tunnel.at
>> index a74a67aa8123..e08fd1e048f8 100644
>> --- a/tests/tunnel.at
>> +++ b/tests/tunnel.at
>> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
>>      p2 2/2: (dummy)
>>  ])
>>  
>> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>>  
>>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
>> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>>  ])
>>  
>>  OVS_VSWITCHD_STOP
>>
> 
> 
> travis ci results link
> https://travis-ci.org/github/roidayan/ovs/builds/698910052
> 

+ilya
Simon Horman June 16, 2020, 3:48 p.m. UTC | #3
On Tue, Jun 16, 2020 at 05:06:49PM +0300, Roi Dayan wrote:
> 
> 
> On 2020-06-16 4:03 PM, Roi Dayan wrote:
> > The cited commit intended to add tc support for masking tunnel src/dst
> > ips and ports. It's not possible to do tunnel ports masking with
> > openflow rules and the default mask for tunnel ports set to 0 in
> > tnl_wc_init(), unlike tunnel ports default mask which is full mask.
> > So instead of never passing tunnel ports to tc, revert the changes
> > to tunnel ports to always pass the tunnel port.
> > In sw classification is done by the kernel, but for hw we must match
> > the tunnel dst port.
> > 
> > Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
> > Signed-off-by: Roi Dayan <roid@mellanox.com>
> > Reviewed-by: Eli Britstein <elibr@mellanox.com>
> > ---
> >  NEWS                        |  2 --
> >  include/openvswitch/match.h |  3 ---
> >  lib/match.c                 | 13 -------------
> >  lib/netdev-offload-tc.c     | 13 ++-----------
> >  lib/tc.c                    | 28 ++--------------------------
> >  tests/tunnel.at             |  4 ++--
> >  6 files changed, 6 insertions(+), 57 deletions(-)
> > 
> > diff --git a/NEWS b/NEWS
> > index 88b273a0af89..22cacda20ac7 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -19,8 +19,6 @@ Post-v2.13.0
> >     - Tunnels: TC Flower offload
> >       * Tunnel Local endpoint address masked match are supported.
> >       * Tunnel Romte endpoint address masked match are supported.
> > -     * Tunnel Local endpoint ports masked match are supported.
> > -     * Tunnel Romte endpoint ports masked match are supported.
> >  
> >  
> >  v2.13.0 - 14 Feb 2020
> > diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> > index 9e480318e2b3..2e8812048e86 100644
> > --- a/include/openvswitch/match.h
> > +++ b/include/openvswitch/match.h
> > @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
> >  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
> >  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
> >  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
> > -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
> > -void match_set_tun_tp_src_masked(struct match *match,
> > -                                 ovs_be16 port, ovs_be16 mask);
> >  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
> >  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
> >  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
> > diff --git a/lib/match.c b/lib/match.c
> > index a77554851146..ba716579d8ec 100644
> > --- a/lib/match.c
> > +++ b/lib/match.c
> > @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> >  }
> >  
> >  void
> > -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
> > -{
> > -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
> > -}
> > -
> > -void
> > -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> > -{
> > -    match->wc.masks.tunnel.tp_src = mask;
> > -    match->flow.tunnel.tp_src = port & mask;
> > -}
> > -
> > -void
> >  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
> >  {
> >      match->wc.masks.tunnel.gbp_id = mask;
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index aa6d22e74f29..258d31f54b08 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> >              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
> >                                       flower->mask.tunnel.ttl);
> >          }
> > -        if (flower->mask.tunnel.tp_dst) {
> > -            match_set_tun_tp_dst_masked(match,
> > -                                        flower->key.tunnel.tp_dst,
> > -                                        flower->mask.tunnel.tp_dst);
> > -        }
> > -        if (flower->mask.tunnel.tp_src) {
> > -            match_set_tun_tp_src_masked(match,
> > -                                        flower->key.tunnel.tp_src,
> > -                                        flower->mask.tunnel.tp_src);
> > +        if (flower->key.tunnel.tp_dst) {
> > +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
> >          }
> >          if (flower->key.tunnel.metadata.present.len) {
> >              flower_tun_opt_to_match(match, flower);
> > @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
> >          flower.mask.tunnel.tos = tnl_mask->ip_tos;
> >          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
> > -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
> > -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
> >          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
> >          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
> >          flower.tunnel = true;
> > diff --git a/lib/tc.c b/lib/tc.c
> > index 29b4328d8bfc..c2ab77553306 100644
> > --- a/lib/tc.c
> > +++ b/lib/tc.c
> > @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
> >                                             .optional = true, },
> >      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
> >                                            .optional = true, },
> > -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
> > -                                          .optional = true, },
> > -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
> > -                                               .optional = true, },
> > -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
> > -                                               .optional = true, },
> >      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
> >      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
> >      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
> > @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
> >          flower->key.tunnel.ipv6.ipv6_dst =
> >              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
> >      }
> > -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
> > -        flower->mask.tunnel.tp_src =
> > -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
> > -        flower->key.tunnel.tp_src =
> > -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
> > -    }
> > -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
> > -        flower->mask.tunnel.tp_dst =
> > -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
> > +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
> >          flower->key.tunnel.tp_dst =
> >              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
> >      }
> > @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
> >      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
> >      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
> >      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
> > -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
> > -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
> >      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
> > -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
> >      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
> >      uint8_t tos = flower->key.tunnel.tos;
> >      uint8_t ttl = flower->key.tunnel.ttl;
> > @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
> >          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
> >          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
> >      }
> > -    if (tp_dst_mask) {
> > -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
> > -                        tp_dst_mask);
> > +    if (tp_dst) {
> >          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
> >      }
> > -    if (tp_src_mask) {
> > -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
> > -                        tp_src_mask);
> > -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
> > -    }
> >      if (id_mask) {
> >          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
> >      }
> > diff --git a/tests/tunnel.at b/tests/tunnel.at
> > index a74a67aa8123..e08fd1e048f8 100644
> > --- a/tests/tunnel.at
> > +++ b/tests/tunnel.at
> > @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
> >      p2 2/2: (dummy)
> >  ])
> >  
> > -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> > +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> >  
> >  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
> > -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> > +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> >  ])
> >  
> >  OVS_VSWITCHD_STOP
> > 
> 
> 
> travis ci results link
> https://travis-ci.org/github/roidayan/ovs/builds/698910052

Thanks. Lets wait a day to see if anyone comments on this patch.
Tonghao Zhang June 17, 2020, 5:48 a.m. UTC | #4
On Tue, Jun 16, 2020 at 9:05 PM Roi Dayan <roid@mellanox.com> wrote:
>
> The cited commit intended to add tc support for masking tunnel src/dst
> ips and ports. It's not possible to do tunnel ports masking with
> openflow rules and the default mask for tunnel ports set to 0 in
> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
> So instead of never passing tunnel ports to tc, revert the changes
> to tunnel ports to always pass the tunnel port.
> In sw classification is done by the kernel, but for hw we must match
> the tunnel dst port.
Hi Roi
I use the "ovs-appctl dpctl/add-flow" to install rules with tunnel
src/dst port masked to tc dp.
And I didn't find that feature affects openflow layer. "make check"
all test suite were passed.
If hw we must match tunnel dst port, can we change the tnl_wc_init. I
test it ok.

diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index 03f0ab76562a..5145a6d54e80 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -381,8 +381,8 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards *wc)
         wc->masks.tunnel.ip_ttl = 0;
         /* The tp_src and tp_dst members in flow_tnl are set to be always
          * wildcarded, not to unwildcard them here. */
-        wc->masks.tunnel.tp_src = 0;
-        wc->masks.tunnel.tp_dst = 0;
+        wc->masks.tunnel.tp_src = OVS_BE16_MAX;
+        wc->masks.tunnel.tp_dst = OVS_BE16_MAX;

         if (is_ip_any(flow)
             && IP_ECN_is_ce(flow->tunnel.ip_tos)) {

> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Eli Britstein <elibr@mellanox.com>
> ---
>  NEWS                        |  2 --
>  include/openvswitch/match.h |  3 ---
>  lib/match.c                 | 13 -------------
>  lib/netdev-offload-tc.c     | 13 ++-----------
>  lib/tc.c                    | 28 ++--------------------------
>  tests/tunnel.at             |  4 ++--
>  6 files changed, 6 insertions(+), 57 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 88b273a0af89..22cacda20ac7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -19,8 +19,6 @@ Post-v2.13.0
>     - Tunnels: TC Flower offload
>       * Tunnel Local endpoint address masked match are supported.
>       * Tunnel Romte endpoint address masked match are supported.
> -     * Tunnel Local endpoint ports masked match are supported.
> -     * Tunnel Romte endpoint ports masked match are supported.
>
>
>  v2.13.0 - 14 Feb 2020
> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> index 9e480318e2b3..2e8812048e86 100644
> --- a/include/openvswitch/match.h
> +++ b/include/openvswitch/match.h
> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
> -void match_set_tun_tp_src_masked(struct match *match,
> -                                 ovs_be16 port, ovs_be16 mask);
>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
> diff --git a/lib/match.c b/lib/match.c
> index a77554851146..ba716579d8ec 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>  }
>
>  void
> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
> -{
> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
> -}
> -
> -void
> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> -{
> -    match->wc.masks.tunnel.tp_src = mask;
> -    match->flow.tunnel.tp_src = port & mask;
> -}
> -
> -void
>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
>  {
>      match->wc.masks.tunnel.gbp_id = mask;
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index aa6d22e74f29..258d31f54b08 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
>                                       flower->mask.tunnel.ttl);
>          }
> -        if (flower->mask.tunnel.tp_dst) {
> -            match_set_tun_tp_dst_masked(match,
> -                                        flower->key.tunnel.tp_dst,
> -                                        flower->mask.tunnel.tp_dst);
> -        }
> -        if (flower->mask.tunnel.tp_src) {
> -            match_set_tun_tp_src_masked(match,
> -                                        flower->key.tunnel.tp_src,
> -                                        flower->mask.tunnel.tp_src);
> +        if (flower->key.tunnel.tp_dst) {
> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
>          }
>          if (flower->key.tunnel.metadata.present.len) {
>              flower_tun_opt_to_match(match, flower);
> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
>          flower.tunnel = true;
> diff --git a/lib/tc.c b/lib/tc.c
> index 29b4328d8bfc..c2ab77553306 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
>                                             .optional = true, },
>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
>                                            .optional = true, },
> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
> -                                          .optional = true, },
> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
> -                                               .optional = true, },
> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
> -                                               .optional = true, },
>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>          flower->key.tunnel.ipv6.ipv6_dst =
>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
>      }
> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
> -        flower->mask.tunnel.tp_src =
> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
> -        flower->key.tunnel.tp_src =
> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
> -    }
> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
> -        flower->mask.tunnel.tp_dst =
> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
>          flower->key.tunnel.tp_dst =
>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>      }
> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
>      uint8_t tos = flower->key.tunnel.tos;
>      uint8_t ttl = flower->key.tunnel.ttl;
> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
>      }
> -    if (tp_dst_mask) {
> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
> -                        tp_dst_mask);
> +    if (tp_dst) {
>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
>      }
> -    if (tp_src_mask) {
> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
> -                        tp_src_mask);
> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
> -    }
>      if (id_mask) {
>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>      }
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index a74a67aa8123..e08fd1e048f8 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
>      p2 2/2: (dummy)
>  ])
>
> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>
>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>  ])
>
>  OVS_VSWITCHD_STOP
> --
> 2.8.4
>
Roi Dayan June 17, 2020, 7:20 a.m. UTC | #5
On 2020-06-17 8:48 AM, Tonghao Zhang wrote:
> On Tue, Jun 16, 2020 at 9:05 PM Roi Dayan <roid@mellanox.com> wrote:
>>
>> The cited commit intended to add tc support for masking tunnel src/dst
>> ips and ports. It's not possible to do tunnel ports masking with
>> openflow rules and the default mask for tunnel ports set to 0 in
>> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
>> So instead of never passing tunnel ports to tc, revert the changes
>> to tunnel ports to always pass the tunnel port.
>> In sw classification is done by the kernel, but for hw we must match
>> the tunnel dst port.
> Hi Roi
> I use the "ovs-appctl dpctl/add-flow" to install rules with tunnel
> src/dst port masked to tc dp.
> And I didn't find that feature affects openflow layer. "make check"
> all test suite were passed.
> If hw we must match tunnel dst port, can we change the tnl_wc_init. I
> test it ok.

dpctl used to debug and not strict like openflows. you can do a lot of stuff
in it to break ovs normal operation, e.g. not aging rules, not adding new rules.
But with openflow rules, you cannot actually control masking of tunnel ports.
"make check" is more of unit testing. its spawning new ovs process for a
single test/check so I don't think it's valid check for this case.

also, I'm not sure about changing tnl_wc_init(), because of the specific comment
about not to change it. also src port should stay 0.
otherwise you will get a lot of rules because the src port is random port opened
for a connection. 

from the original commit "8b7ea2d Extend OVS IPFIX exporter to export tunnel headers"
I see there is another comment in other place in the code expecting default 0 for ports.
* The protocol identifier of DPIF_IPFIX_TUNNEL_IPSEC_GRE is IPPROTO_GRE,
* and both tp_src and tp_dst are zero.

> 
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 03f0ab76562a..5145a6d54e80 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -381,8 +381,8 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards *wc)
>          wc->masks.tunnel.ip_ttl = 0;
>          /* The tp_src and tp_dst members in flow_tnl are set to be always
>           * wildcarded, not to unwildcard them here. */
> -        wc->masks.tunnel.tp_src = 0;
> -        wc->masks.tunnel.tp_dst = 0;
> +        wc->masks.tunnel.tp_src = OVS_BE16_MAX;
> +        wc->masks.tunnel.tp_dst = OVS_BE16_MAX;
> 
>          if (is_ip_any(flow)
>              && IP_ECN_is_ce(flow->tunnel.ip_tos)) {
> 
>> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>> Reviewed-by: Eli Britstein <elibr@mellanox.com>
>> ---
>>  NEWS                        |  2 --
>>  include/openvswitch/match.h |  3 ---
>>  lib/match.c                 | 13 -------------
>>  lib/netdev-offload-tc.c     | 13 ++-----------
>>  lib/tc.c                    | 28 ++--------------------------
>>  tests/tunnel.at             |  4 ++--
>>  6 files changed, 6 insertions(+), 57 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 88b273a0af89..22cacda20ac7 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -19,8 +19,6 @@ Post-v2.13.0
>>     - Tunnels: TC Flower offload
>>       * Tunnel Local endpoint address masked match are supported.
>>       * Tunnel Romte endpoint address masked match are supported.
>> -     * Tunnel Local endpoint ports masked match are supported.
>> -     * Tunnel Romte endpoint ports masked match are supported.
>>
>>
>>  v2.13.0 - 14 Feb 2020
>> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
>> index 9e480318e2b3..2e8812048e86 100644
>> --- a/include/openvswitch/match.h
>> +++ b/include/openvswitch/match.h
>> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
>>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
>>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
>>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
>> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
>> -void match_set_tun_tp_src_masked(struct match *match,
>> -                                 ovs_be16 port, ovs_be16 mask);
>>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
>>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
>>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
>> diff --git a/lib/match.c b/lib/match.c
>> index a77554851146..ba716579d8ec 100644
>> --- a/lib/match.c
>> +++ b/lib/match.c
>> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>>  }
>>
>>  void
>> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
>> -{
>> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
>> -}
>> -
>> -void
>> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>> -{
>> -    match->wc.masks.tunnel.tp_src = mask;
>> -    match->flow.tunnel.tp_src = port & mask;
>> -}
>> -
>> -void
>>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
>>  {
>>      match->wc.masks.tunnel.gbp_id = mask;
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index aa6d22e74f29..258d31f54b08 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
>>                                       flower->mask.tunnel.ttl);
>>          }
>> -        if (flower->mask.tunnel.tp_dst) {
>> -            match_set_tun_tp_dst_masked(match,
>> -                                        flower->key.tunnel.tp_dst,
>> -                                        flower->mask.tunnel.tp_dst);
>> -        }
>> -        if (flower->mask.tunnel.tp_src) {
>> -            match_set_tun_tp_src_masked(match,
>> -                                        flower->key.tunnel.tp_src,
>> -                                        flower->mask.tunnel.tp_src);
>> +        if (flower->key.tunnel.tp_dst) {
>> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
>>          }
>>          if (flower->key.tunnel.metadata.present.len) {
>>              flower_tun_opt_to_match(match, flower);
>> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
>>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
>>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
>> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
>> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
>>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
>>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
>>          flower.tunnel = true;
>> diff --git a/lib/tc.c b/lib/tc.c
>> index 29b4328d8bfc..c2ab77553306 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
>>                                             .optional = true, },
>>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
>>                                            .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
>> -                                          .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
>> -                                               .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
>> -                                               .optional = true, },
>>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
>>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
>>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
>> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>>          flower->key.tunnel.ipv6.ipv6_dst =
>>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
>>      }
>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
>> -        flower->mask.tunnel.tp_src =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
>> -        flower->key.tunnel.tp_src =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
>> -    }
>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
>> -        flower->mask.tunnel.tp_dst =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
>> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
>>          flower->key.tunnel.tp_dst =
>>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>>      }
>> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
>>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
>>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
>> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
>> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
>>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
>> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
>>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
>>      uint8_t tos = flower->key.tunnel.tos;
>>      uint8_t ttl = flower->key.tunnel.ttl;
>> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
>>      }
>> -    if (tp_dst_mask) {
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
>> -                        tp_dst_mask);
>> +    if (tp_dst) {
>>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
>>      }
>> -    if (tp_src_mask) {
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
>> -                        tp_src_mask);
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
>> -    }
>>      if (id_mask) {
>>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>>      }
>> diff --git a/tests/tunnel.at b/tests/tunnel.at
>> index a74a67aa8123..e08fd1e048f8 100644
>> --- a/tests/tunnel.at
>> +++ b/tests/tunnel.at
>> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
>>      p2 2/2: (dummy)
>>  ])
>>
>> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>>
>>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
>> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>>  ])
>>
>>  OVS_VSWITCHD_STOP
>> --
>> 2.8.4
>>
> 
>
Tonghao Zhang June 17, 2020, 10:15 a.m. UTC | #6
On Wed, Jun 17, 2020 at 3:20 PM Roi Dayan <roid@mellanox.com> wrote:
>
>
>
> On 2020-06-17 8:48 AM, Tonghao Zhang wrote:
> > On Tue, Jun 16, 2020 at 9:05 PM Roi Dayan <roid@mellanox.com> wrote:
> >>
> >> The cited commit intended to add tc support for masking tunnel src/dst
> >> ips and ports. It's not possible to do tunnel ports masking with
> >> openflow rules and the default mask for tunnel ports set to 0 in
> >> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
> >> So instead of never passing tunnel ports to tc, revert the changes
> >> to tunnel ports to always pass the tunnel port.
> >> In sw classification is done by the kernel, but for hw we must match
> >> the tunnel dst port.
> > Hi Roi
> > I use the "ovs-appctl dpctl/add-flow" to install rules with tunnel
> > src/dst port masked to tc dp.
> > And I didn't find that feature affects openflow layer. "make check"
> > all test suite were passed.
> > If hw we must match tunnel dst port, can we change the tnl_wc_init. I
> > test it ok.
>
> dpctl used to debug and not strict like openflows. you can do a lot of stuff
> in it to break ovs normal operation, e.g. not aging rules, not adding new rules.
> But with openflow rules, you cannot actually control masking of tunnel ports.
> "make check" is more of unit testing. its spawning new ovs process for a
> single test/check so I don't think it's valid check for this case.
>
> also, I'm not sure about changing tnl_wc_init(), because of the specific comment
> about not to change it. also src port should stay 0.
> otherwise you will get a lot of rules because the src port is random port opened
> for a connection.
>
> from the original commit "8b7ea2d Extend OVS IPFIX exporter to export tunnel headers"
> I see there is another comment in other place in the code expecting default 0 for ports.
> * The protocol identifier of DPIF_IPFIX_TUNNEL_IPSEC_GRE is IPPROTO_GRE,
> * and both tp_src and tp_dst are zero.
Thanks Roi, I got it.
> >
> > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> > index 03f0ab76562a..5145a6d54e80 100644
> > --- a/ofproto/tunnel.c
> > +++ b/ofproto/tunnel.c
> > @@ -381,8 +381,8 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards *wc)
> >          wc->masks.tunnel.ip_ttl = 0;
> >          /* The tp_src and tp_dst members in flow_tnl are set to be always
> >           * wildcarded, not to unwildcard them here. */
> > -        wc->masks.tunnel.tp_src = 0;
> > -        wc->masks.tunnel.tp_dst = 0;
> > +        wc->masks.tunnel.tp_src = OVS_BE16_MAX;
> > +        wc->masks.tunnel.tp_dst = OVS_BE16_MAX;
> >
> >          if (is_ip_any(flow)
> >              && IP_ECN_is_ce(flow->tunnel.ip_tos)) {
> >
> >> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
> >> Signed-off-by: Roi Dayan <roid@mellanox.com>
> >> Reviewed-by: Eli Britstein <elibr@mellanox.com>
> >> ---
> >>  NEWS                        |  2 --
> >>  include/openvswitch/match.h |  3 ---
> >>  lib/match.c                 | 13 -------------
> >>  lib/netdev-offload-tc.c     | 13 ++-----------
> >>  lib/tc.c                    | 28 ++--------------------------
> >>  tests/tunnel.at             |  4 ++--
> >>  6 files changed, 6 insertions(+), 57 deletions(-)
> >>
> >> diff --git a/NEWS b/NEWS
> >> index 88b273a0af89..22cacda20ac7 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -19,8 +19,6 @@ Post-v2.13.0
> >>     - Tunnels: TC Flower offload
> >>       * Tunnel Local endpoint address masked match are supported.
> >>       * Tunnel Romte endpoint address masked match are supported.
> >> -     * Tunnel Local endpoint ports masked match are supported.
> >> -     * Tunnel Romte endpoint ports masked match are supported.
> >>
> >>
> >>  v2.13.0 - 14 Feb 2020
> >> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> >> index 9e480318e2b3..2e8812048e86 100644
> >> --- a/include/openvswitch/match.h
> >> +++ b/include/openvswitch/match.h
> >> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
> >>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
> >>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
> >>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
> >> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
> >> -void match_set_tun_tp_src_masked(struct match *match,
> >> -                                 ovs_be16 port, ovs_be16 mask);
> >>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
> >>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
> >>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
> >> diff --git a/lib/match.c b/lib/match.c
> >> index a77554851146..ba716579d8ec 100644
> >> --- a/lib/match.c
> >> +++ b/lib/match.c
> >> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> >>  }
> >>
> >>  void
> >> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
> >> -{
> >> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
> >> -}
> >> -
> >> -void
> >> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> >> -{
> >> -    match->wc.masks.tunnel.tp_src = mask;
> >> -    match->flow.tunnel.tp_src = port & mask;
> >> -}
> >> -
> >> -void
> >>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
> >>  {
> >>      match->wc.masks.tunnel.gbp_id = mask;
> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> >> index aa6d22e74f29..258d31f54b08 100644
> >> --- a/lib/netdev-offload-tc.c
> >> +++ b/lib/netdev-offload-tc.c
> >> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> >>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
> >>                                       flower->mask.tunnel.ttl);
> >>          }
> >> -        if (flower->mask.tunnel.tp_dst) {
> >> -            match_set_tun_tp_dst_masked(match,
> >> -                                        flower->key.tunnel.tp_dst,
> >> -                                        flower->mask.tunnel.tp_dst);
> >> -        }
> >> -        if (flower->mask.tunnel.tp_src) {
> >> -            match_set_tun_tp_src_masked(match,
> >> -                                        flower->key.tunnel.tp_src,
> >> -                                        flower->mask.tunnel.tp_src);
> >> +        if (flower->key.tunnel.tp_dst) {
> >> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
> >>          }
> >>          if (flower->key.tunnel.metadata.present.len) {
> >>              flower_tun_opt_to_match(match, flower);
> >> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
> >>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
> >>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
> >> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
> >> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
> >>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
> >>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
> >>          flower.tunnel = true;
> >> diff --git a/lib/tc.c b/lib/tc.c
> >> index 29b4328d8bfc..c2ab77553306 100644
> >> --- a/lib/tc.c
> >> +++ b/lib/tc.c
> >> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
> >>                                             .optional = true, },
> >>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
> >>                                            .optional = true, },
> >> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
> >> -                                          .optional = true, },
> >> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
> >> -                                               .optional = true, },
> >> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
> >> -                                               .optional = true, },
> >>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
> >>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
> >>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
> >> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
> >>          flower->key.tunnel.ipv6.ipv6_dst =
> >>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
> >>      }
> >> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
> >> -        flower->mask.tunnel.tp_src =
> >> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
> >> -        flower->key.tunnel.tp_src =
> >> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
> >> -    }
> >> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
> >> -        flower->mask.tunnel.tp_dst =
> >> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
> >> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
> >>          flower->key.tunnel.tp_dst =
> >>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
> >>      }
> >> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
> >>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
> >>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
> >>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
> >> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
> >> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
> >>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
> >> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
> >>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
> >>      uint8_t tos = flower->key.tunnel.tos;
> >>      uint8_t ttl = flower->key.tunnel.ttl;
> >> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
> >>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
> >>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
> >>      }
> >> -    if (tp_dst_mask) {
> >> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
> >> -                        tp_dst_mask);
> >> +    if (tp_dst) {
> >>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
> >>      }
> >> -    if (tp_src_mask) {
> >> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
> >> -                        tp_src_mask);
> >> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
> >> -    }
> >>      if (id_mask) {
> >>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
> >>      }
> >> diff --git a/tests/tunnel.at b/tests/tunnel.at
> >> index a74a67aa8123..e08fd1e048f8 100644
> >> --- a/tests/tunnel.at
> >> +++ b/tests/tunnel.at
> >> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
> >>      p2 2/2: (dummy)
> >>  ])
> >>
> >> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> >> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> >>
> >>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
> >> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> >> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> >>  ])
> >>
> >>  OVS_VSWITCHD_STOP
> >> --
> >> 2.8.4
> >>
> >
> >
Roi Dayan June 18, 2020, 4:36 p.m. UTC | #7
On 2020-06-16 6:48 PM, Simon Horman wrote:
> On Tue, Jun 16, 2020 at 05:06:49PM +0300, Roi Dayan wrote:
>>
>>
>> On 2020-06-16 4:03 PM, Roi Dayan wrote:
>>> The cited commit intended to add tc support for masking tunnel src/dst
>>> ips and ports. It's not possible to do tunnel ports masking with
>>> openflow rules and the default mask for tunnel ports set to 0 in
>>> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
>>> So instead of never passing tunnel ports to tc, revert the changes
>>> to tunnel ports to always pass the tunnel port.
>>> In sw classification is done by the kernel, but for hw we must match
>>> the tunnel dst port.
>>>
>>> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>>> Reviewed-by: Eli Britstein <elibr@mellanox.com>
>>> ---
>>>  NEWS                        |  2 --
>>>  include/openvswitch/match.h |  3 ---
>>>  lib/match.c                 | 13 -------------
>>>  lib/netdev-offload-tc.c     | 13 ++-----------
>>>  lib/tc.c                    | 28 ++--------------------------
>>>  tests/tunnel.at             |  4 ++--
>>>  6 files changed, 6 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index 88b273a0af89..22cacda20ac7 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -19,8 +19,6 @@ Post-v2.13.0
>>>     - Tunnels: TC Flower offload
>>>       * Tunnel Local endpoint address masked match are supported.
>>>       * Tunnel Romte endpoint address masked match are supported.
>>> -     * Tunnel Local endpoint ports masked match are supported.
>>> -     * Tunnel Romte endpoint ports masked match are supported.
>>>  
>>>  
>>>  v2.13.0 - 14 Feb 2020
>>> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
>>> index 9e480318e2b3..2e8812048e86 100644
>>> --- a/include/openvswitch/match.h
>>> +++ b/include/openvswitch/match.h
>>> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
>>>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
>>>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
>>>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
>>> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
>>> -void match_set_tun_tp_src_masked(struct match *match,
>>> -                                 ovs_be16 port, ovs_be16 mask);
>>>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
>>>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
>>>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
>>> diff --git a/lib/match.c b/lib/match.c
>>> index a77554851146..ba716579d8ec 100644
>>> --- a/lib/match.c
>>> +++ b/lib/match.c
>>> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>>>  }
>>>  
>>>  void
>>> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
>>> -{
>>> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
>>> -}
>>> -
>>> -void
>>> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>>> -{
>>> -    match->wc.masks.tunnel.tp_src = mask;
>>> -    match->flow.tunnel.tp_src = port & mask;
>>> -}
>>> -
>>> -void
>>>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
>>>  {
>>>      match->wc.masks.tunnel.gbp_id = mask;
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index aa6d22e74f29..258d31f54b08 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>>>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
>>>                                       flower->mask.tunnel.ttl);
>>>          }
>>> -        if (flower->mask.tunnel.tp_dst) {
>>> -            match_set_tun_tp_dst_masked(match,
>>> -                                        flower->key.tunnel.tp_dst,
>>> -                                        flower->mask.tunnel.tp_dst);
>>> -        }
>>> -        if (flower->mask.tunnel.tp_src) {
>>> -            match_set_tun_tp_src_masked(match,
>>> -                                        flower->key.tunnel.tp_src,
>>> -                                        flower->mask.tunnel.tp_src);
>>> +        if (flower->key.tunnel.tp_dst) {
>>> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
>>>          }
>>>          if (flower->key.tunnel.metadata.present.len) {
>>>              flower_tun_opt_to_match(match, flower);
>>> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
>>>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
>>>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
>>> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
>>> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
>>>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
>>>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
>>>          flower.tunnel = true;
>>> diff --git a/lib/tc.c b/lib/tc.c
>>> index 29b4328d8bfc..c2ab77553306 100644
>>> --- a/lib/tc.c
>>> +++ b/lib/tc.c
>>> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
>>>                                             .optional = true, },
>>>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
>>>                                            .optional = true, },
>>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
>>> -                                          .optional = true, },
>>> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
>>> -                                               .optional = true, },
>>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
>>> -                                               .optional = true, },
>>>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
>>>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
>>>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
>>> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>>>          flower->key.tunnel.ipv6.ipv6_dst =
>>>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
>>>      }
>>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
>>> -        flower->mask.tunnel.tp_src =
>>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
>>> -        flower->key.tunnel.tp_src =
>>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
>>> -    }
>>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
>>> -        flower->mask.tunnel.tp_dst =
>>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
>>> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
>>>          flower->key.tunnel.tp_dst =
>>>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>>>      }
>>> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
>>>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
>>>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
>>> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
>>> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
>>>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
>>> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
>>>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
>>>      uint8_t tos = flower->key.tunnel.tos;
>>>      uint8_t ttl = flower->key.tunnel.ttl;
>>> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
>>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
>>>      }
>>> -    if (tp_dst_mask) {
>>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
>>> -                        tp_dst_mask);
>>> +    if (tp_dst) {
>>>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
>>>      }
>>> -    if (tp_src_mask) {
>>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
>>> -                        tp_src_mask);
>>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
>>> -    }
>>>      if (id_mask) {
>>>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>>>      }
>>> diff --git a/tests/tunnel.at b/tests/tunnel.at
>>> index a74a67aa8123..e08fd1e048f8 100644
>>> --- a/tests/tunnel.at
>>> +++ b/tests/tunnel.at
>>> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
>>>      p2 2/2: (dummy)
>>>  ])
>>>  
>>> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>>> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>>>  
>>>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
>>> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>>> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>>>  ])
>>>  
>>>  OVS_VSWITCHD_STOP
>>>
>>
>>
>> travis ci results link
>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Froidayan%2Fovs%2Fbuilds%2F698910052&amp;data=02%7C01%7Croid%40mellanox.com%7C6322e40390a746863d1508d8120cc8e8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637279193414131030&amp;sdata=PYrnAiLLFStqTsZXaXZFPAoL1ed68q8TxcB5Pxn1IqE%3D&amp;reserved=0
> 
> Thanks. Lets wait a day to see if anyone comments on this patch.
> 

thanks. can we merge this or should we wait more?
Simon Horman June 19, 2020, 6:52 a.m. UTC | #8
On Thu, Jun 18, 2020 at 07:36:52PM +0300, Roi Dayan wrote:
> 
> 
> On 2020-06-16 6:48 PM, Simon Horman wrote:
> > On Tue, Jun 16, 2020 at 05:06:49PM +0300, Roi Dayan wrote:
> >>
> >>
> >> On 2020-06-16 4:03 PM, Roi Dayan wrote:
> >>> The cited commit intended to add tc support for masking tunnel src/dst
> >>> ips and ports. It's not possible to do tunnel ports masking with
> >>> openflow rules and the default mask for tunnel ports set to 0 in
> >>> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
> >>> So instead of never passing tunnel ports to tc, revert the changes
> >>> to tunnel ports to always pass the tunnel port.
> >>> In sw classification is done by the kernel, but for hw we must match
> >>> the tunnel dst port.
> >>>
> >>> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
> >>> Signed-off-by: Roi Dayan <roid@mellanox.com>
> >>> Reviewed-by: Eli Britstein <elibr@mellanox.com>
> >>> ---
> >>>  NEWS                        |  2 --
> >>>  include/openvswitch/match.h |  3 ---
> >>>  lib/match.c                 | 13 -------------
> >>>  lib/netdev-offload-tc.c     | 13 ++-----------
> >>>  lib/tc.c                    | 28 ++--------------------------
> >>>  tests/tunnel.at             |  4 ++--
> >>>  6 files changed, 6 insertions(+), 57 deletions(-)
> >>>
> >>> diff --git a/NEWS b/NEWS
> >>> index 88b273a0af89..22cacda20ac7 100644
> >>> --- a/NEWS
> >>> +++ b/NEWS
> >>> @@ -19,8 +19,6 @@ Post-v2.13.0
> >>>     - Tunnels: TC Flower offload
> >>>       * Tunnel Local endpoint address masked match are supported.
> >>>       * Tunnel Romte endpoint address masked match are supported.
> >>> -     * Tunnel Local endpoint ports masked match are supported.
> >>> -     * Tunnel Romte endpoint ports masked match are supported.
> >>>  
> >>>  
> >>>  v2.13.0 - 14 Feb 2020
> >>> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> >>> index 9e480318e2b3..2e8812048e86 100644
> >>> --- a/include/openvswitch/match.h
> >>> +++ b/include/openvswitch/match.h
> >>> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
> >>>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
> >>>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
> >>>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
> >>> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
> >>> -void match_set_tun_tp_src_masked(struct match *match,
> >>> -                                 ovs_be16 port, ovs_be16 mask);
> >>>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
> >>>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
> >>>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
> >>> diff --git a/lib/match.c b/lib/match.c
> >>> index a77554851146..ba716579d8ec 100644
> >>> --- a/lib/match.c
> >>> +++ b/lib/match.c
> >>> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> >>>  }
> >>>  
> >>>  void
> >>> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
> >>> -{
> >>> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
> >>> -}
> >>> -
> >>> -void
> >>> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
> >>> -{
> >>> -    match->wc.masks.tunnel.tp_src = mask;
> >>> -    match->flow.tunnel.tp_src = port & mask;
> >>> -}
> >>> -
> >>> -void
> >>>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
> >>>  {
> >>>      match->wc.masks.tunnel.gbp_id = mask;
> >>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> >>> index aa6d22e74f29..258d31f54b08 100644
> >>> --- a/lib/netdev-offload-tc.c
> >>> +++ b/lib/netdev-offload-tc.c
> >>> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> >>>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
> >>>                                       flower->mask.tunnel.ttl);
> >>>          }
> >>> -        if (flower->mask.tunnel.tp_dst) {
> >>> -            match_set_tun_tp_dst_masked(match,
> >>> -                                        flower->key.tunnel.tp_dst,
> >>> -                                        flower->mask.tunnel.tp_dst);
> >>> -        }
> >>> -        if (flower->mask.tunnel.tp_src) {
> >>> -            match_set_tun_tp_src_masked(match,
> >>> -                                        flower->key.tunnel.tp_src,
> >>> -                                        flower->mask.tunnel.tp_src);
> >>> +        if (flower->key.tunnel.tp_dst) {
> >>> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
> >>>          }
> >>>          if (flower->key.tunnel.metadata.present.len) {
> >>>              flower_tun_opt_to_match(match, flower);
> >>> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >>>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
> >>>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
> >>>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
> >>> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
> >>> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
> >>>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
> >>>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
> >>>          flower.tunnel = true;
> >>> diff --git a/lib/tc.c b/lib/tc.c
> >>> index 29b4328d8bfc..c2ab77553306 100644
> >>> --- a/lib/tc.c
> >>> +++ b/lib/tc.c
> >>> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
> >>>                                             .optional = true, },
> >>>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
> >>>                                            .optional = true, },
> >>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
> >>> -                                          .optional = true, },
> >>> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
> >>> -                                               .optional = true, },
> >>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
> >>> -                                               .optional = true, },
> >>>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
> >>>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
> >>>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
> >>> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
> >>>          flower->key.tunnel.ipv6.ipv6_dst =
> >>>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
> >>>      }
> >>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
> >>> -        flower->mask.tunnel.tp_src =
> >>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
> >>> -        flower->key.tunnel.tp_src =
> >>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
> >>> -    }
> >>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
> >>> -        flower->mask.tunnel.tp_dst =
> >>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
> >>> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
> >>>          flower->key.tunnel.tp_dst =
> >>>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
> >>>      }
> >>> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
> >>>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
> >>>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
> >>>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
> >>> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
> >>> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
> >>>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
> >>> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
> >>>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
> >>>      uint8_t tos = flower->key.tunnel.tos;
> >>>      uint8_t ttl = flower->key.tunnel.ttl;
> >>> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
> >>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
> >>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
> >>>      }
> >>> -    if (tp_dst_mask) {
> >>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
> >>> -                        tp_dst_mask);
> >>> +    if (tp_dst) {
> >>>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
> >>>      }
> >>> -    if (tp_src_mask) {
> >>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
> >>> -                        tp_src_mask);
> >>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
> >>> -    }
> >>>      if (id_mask) {
> >>>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
> >>>      }
> >>> diff --git a/tests/tunnel.at b/tests/tunnel.at
> >>> index a74a67aa8123..e08fd1e048f8 100644
> >>> --- a/tests/tunnel.at
> >>> +++ b/tests/tunnel.at
> >>> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
> >>>      p2 2/2: (dummy)
> >>>  ])
> >>>  
> >>> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> >>> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
> >>>  
> >>>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
> >>> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> >>> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
> >>>  ])
> >>>  
> >>>  OVS_VSWITCHD_STOP
> >>>
> >>
> >>
> >> travis ci results link
> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Froidayan%2Fovs%2Fbuilds%2F698910052&amp;data=02%7C01%7Croid%40mellanox.com%7C6322e40390a746863d1508d8120cc8e8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637279193414131030&amp;sdata=PYrnAiLLFStqTsZXaXZFPAoL1ed68q8TxcB5Pxn1IqE%3D&amp;reserved=0
> > 
> > Thanks. Lets wait a day to see if anyone comments on this patch.
> > 
> 
> thanks. can we merge this or should we wait more?

Thanks. I've pushed this patch to master.
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 88b273a0af89..22cacda20ac7 100644
--- a/NEWS
+++ b/NEWS
@@ -19,8 +19,6 @@  Post-v2.13.0
    - Tunnels: TC Flower offload
      * Tunnel Local endpoint address masked match are supported.
      * Tunnel Romte endpoint address masked match are supported.
-     * Tunnel Local endpoint ports masked match are supported.
-     * Tunnel Romte endpoint ports masked match are supported.
 
 
 v2.13.0 - 14 Feb 2020
diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
index 9e480318e2b3..2e8812048e86 100644
--- a/include/openvswitch/match.h
+++ b/include/openvswitch/match.h
@@ -105,9 +105,6 @@  void match_set_tun_flags(struct match *match, uint16_t flags);
 void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
 void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
 void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
-void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
-void match_set_tun_tp_src_masked(struct match *match,
-                                 ovs_be16 port, ovs_be16 mask);
 void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
 void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
 void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
diff --git a/lib/match.c b/lib/match.c
index a77554851146..ba716579d8ec 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -294,19 +294,6 @@  match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
 }
 
 void
-match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
-{
-    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
-}
-
-void
-match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
-{
-    match->wc.masks.tunnel.tp_src = mask;
-    match->flow.tunnel.tp_src = port & mask;
-}
-
-void
 match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
 {
     match->wc.masks.tunnel.gbp_id = mask;
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index aa6d22e74f29..258d31f54b08 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -712,15 +712,8 @@  parse_tc_flower_to_match(struct tc_flower *flower,
             match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
                                      flower->mask.tunnel.ttl);
         }
-        if (flower->mask.tunnel.tp_dst) {
-            match_set_tun_tp_dst_masked(match,
-                                        flower->key.tunnel.tp_dst,
-                                        flower->mask.tunnel.tp_dst);
-        }
-        if (flower->mask.tunnel.tp_src) {
-            match_set_tun_tp_src_masked(match,
-                                        flower->key.tunnel.tp_src,
-                                        flower->mask.tunnel.tp_src);
+        if (flower->key.tunnel.tp_dst) {
+            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
         }
         if (flower->key.tunnel.metadata.present.len) {
             flower_tun_opt_to_match(match, flower);
@@ -1470,8 +1463,6 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
         flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
         flower.mask.tunnel.tos = tnl_mask->ip_tos;
         flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
-        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
-        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
         flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
         flower_match_to_tun_opt(&flower, tnl, tnl_mask);
         flower.tunnel = true;
diff --git a/lib/tc.c b/lib/tc.c
index 29b4328d8bfc..c2ab77553306 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -395,12 +395,6 @@  static const struct nl_policy tca_flower_policy[] = {
                                            .optional = true, },
     [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
                                           .optional = true, },
-    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
-                                          .optional = true, },
-    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
-                                               .optional = true, },
-    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
-                                               .optional = true, },
     [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
     [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
     [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
@@ -746,15 +740,7 @@  nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
         flower->key.tunnel.ipv6.ipv6_dst =
             nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
     }
-    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
-        flower->mask.tunnel.tp_src =
-            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
-        flower->key.tunnel.tp_src =
-            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
-    }
-    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
-        flower->mask.tunnel.tp_dst =
-            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
+    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
         flower->key.tunnel.tp_dst =
             nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
     }
@@ -2713,10 +2699,7 @@  nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
     struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
     struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
     struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
-    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
-    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
     ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
-    ovs_be16 tp_src = flower->key.tunnel.tp_src;
     ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
     uint8_t tos = flower->key.tunnel.tos;
     uint8_t ttl = flower->key.tunnel.ttl;
@@ -2748,16 +2731,9 @@  nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
         nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
         nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
     }
-    if (tp_dst_mask) {
-        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
-                        tp_dst_mask);
+    if (tp_dst) {
         nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
     }
-    if (tp_src_mask) {
-        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
-                        tp_src_mask);
-        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
-    }
     if (id_mask) {
         nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
     }
diff --git a/tests/tunnel.at b/tests/tunnel.at
index a74a67aa8123..e08fd1e048f8 100644
--- a/tests/tunnel.at
+++ b/tests/tunnel.at
@@ -123,10 +123,10 @@  AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
     p2 2/2: (dummy)
 ])
 
-AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
+AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
-tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
+tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
 ])
 
 OVS_VSWITCHD_STOP