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

Simon Horman simon.horman at netronome.com
Fri Jun 19 06:52:44 UTC 2020


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 at mellanox.com>
> >>> Reviewed-by: Eli Britstein <elibr at 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&data=02%7C01%7Croid%40mellanox.com%7C6322e40390a746863d1508d8120cc8e8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637279193414131030&sdata=PYrnAiLLFStqTsZXaXZFPAoL1ed68q8TxcB5Pxn1IqE%3D&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.


More information about the dev mailing list