diff mbox series

[ovs-dev,2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel

Message ID 20200518014443.1529-2-xiangxia.m.yue@gmail.com
State Superseded
Headers show
Series [ovs-dev,1/3] dpif-netlink: Generate ufids for installing TC flowers | expand

Commit Message

Tonghao Zhang May 18, 2020, 1:44 a.m. UTC 
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This patch allows users to offload the TC flower rules with tunnel
mask. In some case, mask is useful as wildcards.

For example:
$ ovs-appctl dpctl/add-flow \
    "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
    recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"

Cc: Simon Horman <simon.horman@netronome.com>
Cc: Paul Blakey <paulb@mellanox.com>
Cc: Roi Dayan <roid@mellanox.com>
Cc: Ben Pfaff <blp@ovn.org>
Cc: William Tu <u9012063@gmail.com>
Cc: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 include/openvswitch/match.h |  2 ++
 lib/match.c                 | 13 +++++++++
 lib/netdev-offload-tc.c     | 40 ++++++++++++++++++++------
 lib/tc.c                    | 57 +++++++++++++++++++++++++++++++++----
 tests/tunnel.at             | 22 ++++++++++++++
 5 files changed, 119 insertions(+), 15 deletions(-)

Comments

0-day Robot May 18, 2020, 2:02 a.m. UTC | #1 
Bleep bloop.  Greetings Tonghao Zhang, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 84 characters long (recommended limit is 79)
#38 FILE: include/openvswitch/match.h:109:
void match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask);

Lines checked: 288, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Roi Dayan May 18, 2020, 2:42 p.m. UTC | #2 
On 2020-05-18 4:44 AM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> This patch allows users to offload the TC flower rules with tunnel
> mask. In some case, mask is useful as wildcards.
> 
> For example:
> $ ovs-appctl dpctl/add-flow \
>     "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
>     recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> 
> Cc: Simon Horman <simon.horman@netronome.com>
> Cc: Paul Blakey <paulb@mellanox.com>
> Cc: Roi Dayan <roid@mellanox.com>
> Cc: Ben Pfaff <blp@ovn.org>
> Cc: William Tu <u9012063@gmail.com>
> Cc: Ilya Maximets <i.maximets@ovn.org>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  include/openvswitch/match.h |  2 ++
>  lib/match.c                 | 13 +++++++++
>  lib/netdev-offload-tc.c     | 40 ++++++++++++++++++++------
>  lib/tc.c                    | 57 +++++++++++++++++++++++++++++++++----
>  tests/tunnel.at             | 22 ++++++++++++++
>  5 files changed, 119 insertions(+), 15 deletions(-)
> 
> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> index 8af3b74ed3e0..cbb1a0382a2e 100644
> --- a/include/openvswitch/match.h
> +++ b/include/openvswitch/match.h
> @@ -105,6 +105,8 @@ 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 25c277cc670b..29b25a73bab4 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -293,6 +293,19 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>      match->flow.tunnel.tp_dst = port & 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)
>  {
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 875ebef71941..39cf25f63ce0 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>              match_set_tun_id(match, flower->key.tunnel.id);
>              match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
>          }
> -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> -            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> -            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> -            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
> -            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
> +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> +            flower->mask.tunnel.ipv4.ipv4_src) {
> +            match_set_tun_dst_masked(match,
> +                                     flower->key.tunnel.ipv4.ipv4_dst,
> +                                     flower->mask.tunnel.ipv4.ipv4_dst);
> +            match_set_tun_src_masked(match,
> +                                     flower->key.tunnel.ipv4.ipv4_src,
> +                                     flower->mask.tunnel.ipv4.ipv4_src);
> +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
> +            match_set_tun_ipv6_dst_masked(match,
> +                                          &flower->key.tunnel.ipv6.ipv6_dst,
> +                                          &flower->mask.tunnel.ipv6.ipv6_dst);
> +            match_set_tun_ipv6_src_masked(match,
> +                                          &flower->key.tunnel.ipv6.ipv6_src,
> +                                          &flower->mask.tunnel.ipv6.ipv6_src);
>          }
>          if (flower->key.tunnel.tos) {
>              match_set_tun_tos_masked(match, flower->key.tunnel.tos,
> @@ -649,8 +658,15 @@ 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->key.tunnel.tp_dst) {
> -            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
> +        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.metadata.present.len) {
>              flower_tun_opt_to_match(match, flower);
> @@ -1404,8 +1420,14 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>          flower.key.tunnel.ttl = tnl->ip_ttl;
>          flower.key.tunnel.tp_src = tnl->tp_src;
>          flower.key.tunnel.tp_dst = tnl->tp_dst;
> +        flower.mask.tunnel.ipv4.ipv4_src = tnl_mask->ip_src;
> +        flower.mask.tunnel.ipv4.ipv4_dst = tnl_mask->ip_dst;
> +        flower.mask.tunnel.ipv6.ipv6_src = tnl_mask->ipv6_src;
> +        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 12af0192b614..9ac40f692c17 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -372,6 +372,12 @@ 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,
> @@ -650,22 +656,38 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>          flower->mask.tunnel.id = OVS_BE64_MAX;
>      }
>      if (attrs[TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK]) {
> +        flower->mask.tunnel.ipv4.ipv4_src =
> +            nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK]);
>          flower->key.tunnel.ipv4.ipv4_src =
>              nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_SRC]);
>      }
>      if (attrs[TCA_FLOWER_KEY_ENC_IPV4_DST_MASK]) {
> +        flower->mask.tunnel.ipv4.ipv4_dst =
> +            nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_DST_MASK]);
>          flower->key.tunnel.ipv4.ipv4_dst =
>              nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_DST]);
>      }
>      if (attrs[TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK]) {
> +        flower->mask.tunnel.ipv6.ipv6_src =
> +            nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK]);
>          flower->key.tunnel.ipv6.ipv6_src =
>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_SRC]);
>      }
>      if (attrs[TCA_FLOWER_KEY_ENC_IPV6_DST_MASK]) {
> +        flower->mask.tunnel.ipv6.ipv6_dst =
> +            nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST_MASK]);
>          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_DST_PORT]) {
> +    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]);
>          flower->key.tunnel.tp_dst =
>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>      }
> @@ -2594,11 +2616,18 @@ nl_msg_put_flower_tunnel_opts(struct ofpbuf *request, uint16_t type,
>  static void
>  nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>  {
> +    ovs_be32 ipv4_src_mask = flower->mask.tunnel.ipv4.ipv4_src;
> +    ovs_be32 ipv4_dst_mask = flower->mask.tunnel.ipv4.ipv4_dst;
>      ovs_be32 ipv4_src = flower->key.tunnel.ipv4.ipv4_src;
>      ovs_be32 ipv4_dst = flower->key.tunnel.ipv4.ipv4_dst;
> +    struct in6_addr *ipv6_src_mask = &flower->mask.tunnel.ipv6.ipv6_src;
> +    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;
> @@ -2606,12 +2635,21 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>      uint8_t ttl_mask = flower->mask.tunnel.ttl;
>      ovs_be64 id_mask = flower->mask.tunnel.id;
>  
> -    if (ipv4_dst) {
> -        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_SRC, ipv4_src);
> +    if (ipv4_dst_mask || ipv4_src_mask) {
> +        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_DST_MASK,
> +                        ipv4_dst_mask);
> +        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK,
> +                        ipv4_src_mask);
>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_DST, ipv4_dst);
> -    } else if (!is_all_zeros(ipv6_dst, sizeof *ipv6_dst)) {
> -        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_SRC, ipv6_src);
> +        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_SRC, ipv4_src);
> +    } else if (ipv6_addr_is_set(ipv6_dst_mask) ||
> +               ipv6_addr_is_set(ipv6_src_mask)) {
> +        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_DST_MASK,
> +                            ipv6_dst_mask);
> +        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK,
> +                            ipv6_src_mask);
>          nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_DST, ipv6_dst);
> +        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_SRC, ipv6_src);
>      }
>      if (tos_mask) {
>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TOS, tos);
> @@ -2621,9 +2659,16 @@ 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) {
> +    if (tp_dst_mask) {
> +        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
> +                        tp_dst_mask);
>          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 d65bf4412aa9..d3fdbbe3c4d3 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -110,6 +110,28 @@ Datapath actions: drop
>  OVS_VSWITCHD_STOP(["/dropping tunnel packet marked ECN CE but is not ECN capable/d"])
>  AT_CLEANUP
>  
> +AT_SETUP([tunnel - input with matching tunnel mask])
> +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> +                    options:remote_ip=1.1.1.1 \
> +                    ofport_request=1	\
> +                    -- add-port br0 p2 -- set Interface p2 type=dummy \
> +                    ofport_request=2])
> +
> +AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
> +    br0 65534/100: (dummy-internal)
> +    p1 1/1: (gre: remote_ip=1.1.1.1)
> +    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/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
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([tunnel - output])
>  OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
>                      options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \
> 

Acked-by: Roi Dayan <roid@mellanox.com>
Simon Horman May 29, 2020, 9:06 a.m. UTC | #3 
On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> This patch allows users to offload the TC flower rules with tunnel
> mask. In some case, mask is useful as wildcards.
> 
> For example:
> $ ovs-appctl dpctl/add-flow \
>     "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
>     recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"

Hi,

Sorry for the delay in responding.

I think it would be useful to spell out more clearly in the changelog
what this patch does. From my reading of the code it:

Allows masked match of the following, where previously supported
an exact match was supported:
* Remote (dst) tunnel endpoint address
* Local (src) tunnel endpoint address
* Remote (dst) tunnel endpoint UDP port

And also allows masked match of the following, where previously no
match was supported;
* Local (std) tunnel endpoint UDP port

I think it would also be useful to describe a use-case where this
is used. The command line example (above) is a good start.


Also, not strictly related to this patch.
I think patch series that have more than one patch should
have a cover letter, in this case [PATCH 0/3], describing
the overall aim of the patchset.


The other patches in this series seem fine to me.
Please consider addressing the issues I have raised here
and posting a v2, with all three patches and a cover letter.

...

> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 875ebef71941..39cf25f63ce0 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>              match_set_tun_id(match, flower->key.tunnel.id);
>              match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
>          }
> -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> -            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> -            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> -            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
> -            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
> +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> +            flower->mask.tunnel.ipv4.ipv4_src) {

The change to the if condition seems separate from the change
described in the changelog. What is the use-case for this?

> +            match_set_tun_dst_masked(match,
> +                                     flower->key.tunnel.ipv4.ipv4_dst,
> +                                     flower->mask.tunnel.ipv4.ipv4_dst);
> +            match_set_tun_src_masked(match,
> +                                     flower->key.tunnel.ipv4.ipv4_src,
> +                                     flower->mask.tunnel.ipv4.ipv4_src);
> +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
> +            match_set_tun_ipv6_dst_masked(match,
> +                                          &flower->key.tunnel.ipv6.ipv6_dst,
> +                                          &flower->mask.tunnel.ipv6.ipv6_dst);
> +            match_set_tun_ipv6_src_masked(match,
> +                                          &flower->key.tunnel.ipv6.ipv6_src,
> +                                          &flower->mask.tunnel.ipv6.ipv6_src);
>          }
>          if (flower->key.tunnel.tos) {
>              match_set_tun_tos_masked(match, flower->key.tunnel.tos,

...
Simon Horman May 29, 2020, 9:10 a.m. UTC | #4 
On Sun, May 17, 2020 at 10:02:05PM -0400, 0-day Robot wrote:
> Bleep bloop.  Greetings Tonghao Zhang, I am a robot and I have tried out your patch.
> Thanks for your contribution.
> 
> I encountered some error that I wasn't expecting.  See the details below.
> 
> 
> checkpatch:
> WARNING: Line is 84 characters long (recommended limit is 79)
> #38 FILE: include/openvswitch/match.h:109:
> void match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
> 
> Lines checked: 288, Warnings: 1, Errors: 0
> 
> 
> Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Please consider addressing this (minor) issue when
posting v2 of this series.

Thanks!
Tonghao Zhang June 2, 2020, 8:17 a.m. UTC | #5 
On Fri, May 29, 2020 at 5:06 PM Simon Horman <simon.horman@netronome.com> wrote:
>
> On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > This patch allows users to offload the TC flower rules with tunnel
> > mask. In some case, mask is useful as wildcards.
> >
> > For example:
> > $ ovs-appctl dpctl/add-flow \
> >     "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> >     recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
>
> Hi,
>
> Sorry for the delay in responding.
>
> I think it would be useful to spell out more clearly in the changelog
> what this patch does. From my reading of the code it:
>
> Allows masked match of the following, where previously supported
> an exact match was supported:
> * Remote (dst) tunnel endpoint address
> * Local (src) tunnel endpoint address
> * Remote (dst) tunnel endpoint UDP port
>
> And also allows masked match of the following, where previously no
> match was supported;
> * Local (std) tunnel endpoint UDP port
Ok, Thanks, I will update it in NEWS.
> I think it would also be useful to describe a use-case where this
> is used. The command line example (above) is a good start.
Yes, I will update the commit log and describe a use-case for it.
>
> Also, not strictly related to this patch.
> I think patch series that have more than one patch should
> have a cover letter, in this case [PATCH 0/3], describing
> the overall aim of the patchset.
>
>
> The other patches in this series seem fine to me.
> Please consider addressing the issues I have raised here
> and posting a v2, with all three patches and a cover letter.
>
> ...
>
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index 875ebef71941..39cf25f63ce0 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> >              match_set_tun_id(match, flower->key.tunnel.id);
> >              match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> >          }
> > -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> > -            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > -            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> > -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > -            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
> > -            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
> > +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > +            flower->mask.tunnel.ipv4.ipv4_src) {
>
> The change to the if condition seems separate from the change
> described in the changelog. What is the use-case for this?
I think that may be used for matching the tunnel src packets only
which will be dropped.
because user may don't want that packets sent to the host.
> > +            match_set_tun_dst_masked(match,
> > +                                     flower->key.tunnel.ipv4.ipv4_dst,
> > +                                     flower->mask.tunnel.ipv4.ipv4_dst);
> > +            match_set_tun_src_masked(match,
> > +                                     flower->key.tunnel.ipv4.ipv4_src,
> > +                                     flower->mask.tunnel.ipv4.ipv4_src);
> > +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> > +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
> > +            match_set_tun_ipv6_dst_masked(match,
> > +                                          &flower->key.tunnel.ipv6.ipv6_dst,
> > +                                          &flower->mask.tunnel.ipv6.ipv6_dst);
> > +            match_set_tun_ipv6_src_masked(match,
> > +                                          &flower->key.tunnel.ipv6.ipv6_src,
> > +                                          &flower->mask.tunnel.ipv6.ipv6_src);
> >          }
> >          if (flower->key.tunnel.tos) {
> >              match_set_tun_tos_masked(match, flower->key.tunnel.tos,
>
> ...
Simon Horman June 2, 2020, 8:33 a.m. UTC | #6 
On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> On Fri, May 29, 2020 at 5:06 PM Simon Horman <simon.horman@netronome.com> wrote:
> >
> > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > >
> > > This patch allows users to offload the TC flower rules with tunnel
> > > mask. In some case, mask is useful as wildcards.
> > >
> > > For example:
> > > $ ovs-appctl dpctl/add-flow \
> > >     "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > >     recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> >
> > Hi,
> >
> > Sorry for the delay in responding.
> >
> > I think it would be useful to spell out more clearly in the changelog
> > what this patch does. From my reading of the code it:
> >
> > Allows masked match of the following, where previously supported
> > an exact match was supported:
> > * Remote (dst) tunnel endpoint address
> > * Local (src) tunnel endpoint address
> > * Remote (dst) tunnel endpoint UDP port
> >
> > And also allows masked match of the following, where previously no
> > match was supported;
> > * Local (std) tunnel endpoint UDP port
> Ok, Thanks, I will update it in NEWS.

Thanks. Please also include this information in the changelog.

> > I think it would also be useful to describe a use-case where this
> > is used. The command line example (above) is a good start.
> Yes, I will update the commit log and describe a use-case for it.
> >
> > Also, not strictly related to this patch.
> > I think patch series that have more than one patch should
> > have a cover letter, in this case [PATCH 0/3], describing
> > the overall aim of the patchset.
> >
> >
> > The other patches in this series seem fine to me.
> > Please consider addressing the issues I have raised here
> > and posting a v2, with all three patches and a cover letter.
> >
> > ...
> >
> > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > index 875ebef71941..39cf25f63ce0 100644
> > > --- a/lib/netdev-offload-tc.c
> > > +++ b/lib/netdev-offload-tc.c
> > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> > >              match_set_tun_id(match, flower->key.tunnel.id);
> > >              match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > >          }
> > > -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > -            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > > -            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > > -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> > > -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > -            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
> > > -            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
> > > +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > +            flower->mask.tunnel.ipv4.ipv4_src) {
> >
> > The change to the if condition seems separate from the change
> > described in the changelog. What is the use-case for this?
> I think that may be used for matching the tunnel src packets only
> which will be dropped.
> because user may don't want that packets sent to the host.

I think it would be best to break out this (and the corresponding IPv6
change) into a separate patch with a changelog that describes why
this is being done and, if appropriate, an update to NEWS.

Thanks!

> > > +            match_set_tun_dst_masked(match,
> > > +                                     flower->key.tunnel.ipv4.ipv4_dst,
> > > +                                     flower->mask.tunnel.ipv4.ipv4_dst);
> > > +            match_set_tun_src_masked(match,
> > > +                                     flower->key.tunnel.ipv4.ipv4_src,
> > > +                                     flower->mask.tunnel.ipv4.ipv4_src);
> > > +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> > > +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
> > > +            match_set_tun_ipv6_dst_masked(match,
> > > +                                          &flower->key.tunnel.ipv6.ipv6_dst,
> > > +                                          &flower->mask.tunnel.ipv6.ipv6_dst);
> > > +            match_set_tun_ipv6_src_masked(match,
> > > +                                          &flower->key.tunnel.ipv6.ipv6_src,
> > > +                                          &flower->mask.tunnel.ipv6.ipv6_src);
> > >          }
> > >          if (flower->key.tunnel.tos) {
> > >              match_set_tun_tos_masked(match, flower->key.tunnel.tos,
> >
> > ...
> 
> 
> 
> -- 
> Best regards, Tonghao
Tonghao Zhang June 2, 2020, 8:58 a.m. UTC | #7 
On Tue, Jun 2, 2020 at 4:33 PM Simon Horman <simon.horman@netronome.com> wrote:
>
> On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> > On Fri, May 29, 2020 at 5:06 PM Simon Horman <simon.horman@netronome.com> wrote:
> > >
> > > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > >
> > > > This patch allows users to offload the TC flower rules with tunnel
> > > > mask. In some case, mask is useful as wildcards.
> > > >
> > > > For example:
> > > > $ ovs-appctl dpctl/add-flow \
> > > >     "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > > >     recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> > >
> > > Hi,
> > >
> > > Sorry for the delay in responding.
> > >
> > > I think it would be useful to spell out more clearly in the changelog
> > > what this patch does. From my reading of the code it:
> > >
> > > Allows masked match of the following, where previously supported
> > > an exact match was supported:
> > > * Remote (dst) tunnel endpoint address
> > > * Local (src) tunnel endpoint address
> > > * Remote (dst) tunnel endpoint UDP port
> > >
> > > And also allows masked match of the following, where previously no
> > > match was supported;
> > > * Local (std) tunnel endpoint UDP port
> > Ok, Thanks, I will update it in NEWS.
>
> Thanks. Please also include this information in the changelog.
>
> > > I think it would also be useful to describe a use-case where this
> > > is used. The command line example (above) is a good start.
> > Yes, I will update the commit log and describe a use-case for it.
> > >
> > > Also, not strictly related to this patch.
> > > I think patch series that have more than one patch should
> > > have a cover letter, in this case [PATCH 0/3], describing
> > > the overall aim of the patchset.
> > >
> > >
> > > The other patches in this series seem fine to me.
> > > Please consider addressing the issues I have raised here
> > > and posting a v2, with all three patches and a cover letter.
> > >
> > > ...
> > >
> > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > > index 875ebef71941..39cf25f63ce0 100644
> > > > --- a/lib/netdev-offload-tc.c
> > > > +++ b/lib/netdev-offload-tc.c
> > > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> > > >              match_set_tun_id(match, flower->key.tunnel.id);
> > > >              match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > > >          }
> > > > -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > > -            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > > > -            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > > > -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> > > > -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > > -            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
> > > > -            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
> > > > +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > > +            flower->mask.tunnel.ipv4.ipv4_src) {
> > >
> > > The change to the if condition seems separate from the change
> > > described in the changelog. What is the use-case for this?
> > I think that may be used for matching the tunnel src packets only
> > which will be dropped.
> > because user may don't want that packets sent to the host.
>
> I think it would be best to break out this (and the corresponding IPv6
> change) into a separate patch with a changelog that describes why
> this is being done and, if appropriate, an update to NEWS.
Hi Simon
Did you mean that the two patches as below as a series.
[PATCH 1/3] dpif-netlink: Generate ufids for installing TC flowers
[PATCH 3/3] netdev-offload-tc: Use ipv6_addr_is_set instead of is_all_zeros

and this patch as a separate patch(with changelog)
[PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel

and other question about changelog, which file I should update, or I just update
it in a commit message.

I found the changelog in: debian/changelog
> Thanks!
>
> > > > +            match_set_tun_dst_masked(match,
> > > > +                                     flower->key.tunnel.ipv4.ipv4_dst,
> > > > +                                     flower->mask.tunnel.ipv4.ipv4_dst);
> > > > +            match_set_tun_src_masked(match,
> > > > +                                     flower->key.tunnel.ipv4.ipv4_src,
> > > > +                                     flower->mask.tunnel.ipv4.ipv4_src);
> > > > +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> > > > +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
> > > > +            match_set_tun_ipv6_dst_masked(match,
> > > > +                                          &flower->key.tunnel.ipv6.ipv6_dst,
> > > > +                                          &flower->mask.tunnel.ipv6.ipv6_dst);
> > > > +            match_set_tun_ipv6_src_masked(match,
> > > > +                                          &flower->key.tunnel.ipv6.ipv6_src,
> > > > +                                          &flower->mask.tunnel.ipv6.ipv6_src);
> > > >          }
> > > >          if (flower->key.tunnel.tos) {
> > > >              match_set_tun_tos_masked(match, flower->key.tunnel.tos,
> > >
> > > ...
> >
> >
> >
> > --
> > Best regards, Tonghao
Simon Horman June 2, 2020, 9:34 a.m. UTC | #8 
On Tue, Jun 02, 2020 at 04:58:32PM +0800, Tonghao Zhang wrote:
> On Tue, Jun 2, 2020 at 4:33 PM Simon Horman <simon.horman@netronome.com> wrote:
> >
> > On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> > > On Fri, May 29, 2020 at 5:06 PM Simon Horman <simon.horman@netronome.com> wrote:
> > > >
> > > > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > > >
> > > > > This patch allows users to offload the TC flower rules with tunnel
> > > > > mask. In some case, mask is useful as wildcards.
> > > > >
> > > > > For example:
> > > > > $ ovs-appctl dpctl/add-flow \
> > > > >     "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > > > >     recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> > > >
> > > > Hi,
> > > >
> > > > Sorry for the delay in responding.
> > > >
> > > > I think it would be useful to spell out more clearly in the changelog
> > > > what this patch does. From my reading of the code it:
> > > >
> > > > Allows masked match of the following, where previously supported
> > > > an exact match was supported:
> > > > * Remote (dst) tunnel endpoint address
> > > > * Local (src) tunnel endpoint address
> > > > * Remote (dst) tunnel endpoint UDP port
> > > >
> > > > And also allows masked match of the following, where previously no
> > > > match was supported;
> > > > * Local (std) tunnel endpoint UDP port
> > > Ok, Thanks, I will update it in NEWS.
> >
> > Thanks. Please also include this information in the changelog.
> >
> > > > I think it would also be useful to describe a use-case where this
> > > > is used. The command line example (above) is a good start.
> > > Yes, I will update the commit log and describe a use-case for it.
> > > >
> > > > Also, not strictly related to this patch.
> > > > I think patch series that have more than one patch should
> > > > have a cover letter, in this case [PATCH 0/3], describing
> > > > the overall aim of the patchset.
> > > >
> > > >
> > > > The other patches in this series seem fine to me.
> > > > Please consider addressing the issues I have raised here
> > > > and posting a v2, with all three patches and a cover letter.
> > > >
> > > > ...
> > > >
> > > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > > > index 875ebef71941..39cf25f63ce0 100644
> > > > > --- a/lib/netdev-offload-tc.c
> > > > > +++ b/lib/netdev-offload-tc.c
> > > > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> > > > >              match_set_tun_id(match, flower->key.tunnel.id);
> > > > >              match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > > > >          }
> > > > > -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > > > -            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > > > > -            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > > > > -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> > > > > -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > > > -            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
> > > > > -            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
> > > > > +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > > > +            flower->mask.tunnel.ipv4.ipv4_src) {
> > > >
> > > > The change to the if condition seems separate from the change
> > > > described in the changelog. What is the use-case for this?
> > > I think that may be used for matching the tunnel src packets only
> > > which will be dropped.
> > > because user may don't want that packets sent to the host.
> >
> > I think it would be best to break out this (and the corresponding IPv6
> > change) into a separate patch with a changelog that describes why
> > this is being done and, if appropriate, an update to NEWS.
> Hi Simon
> Did you mean that the two patches as below as a series.
> [PATCH 1/3] dpif-netlink: Generate ufids for installing TC flowers
> [PATCH 3/3] netdev-offload-tc: Use ipv6_addr_is_set instead of is_all_zeros
> 
> and this patch as a separate patch(with changelog)
> [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel

Sorry for not being clear.

I meant expand the series to four patch.
Where the new patch just includes the following changes:

-        if (flower->key.tunnel.ipv4.ipv4_dst) {
+        if (flower->mask.tunnel.ipv4.ipv4_dst ||
+            flower->mask.tunnel.ipv4.ipv4_src) {

...

-        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
-                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
+        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
+                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {

> 
> and other question about changelog, which file I should update, or I just update
> it in a commit message.

Sorry again for not being clear, I meant the commit message.

> 
> I found the changelog in: debian/changelog
> > Thanks!
> >
> > > > > +            match_set_tun_dst_masked(match,
> > > > > +                                     flower->key.tunnel.ipv4.ipv4_dst,
> > > > > +                                     flower->mask.tunnel.ipv4.ipv4_dst);
> > > > > +            match_set_tun_src_masked(match,
> > > > > +                                     flower->key.tunnel.ipv4.ipv4_src,
> > > > > +                                     flower->mask.tunnel.ipv4.ipv4_src);
> > > > > +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> > > > > +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
> > > > > +            match_set_tun_ipv6_dst_masked(match,
> > > > > +                                          &flower->key.tunnel.ipv6.ipv6_dst,
> > > > > +                                          &flower->mask.tunnel.ipv6.ipv6_dst);
> > > > > +            match_set_tun_ipv6_src_masked(match,
> > > > > +                                          &flower->key.tunnel.ipv6.ipv6_src,
> > > > > +                                          &flower->mask.tunnel.ipv6.ipv6_src);
> > > > >          }
> > > > >          if (flower->key.tunnel.tos) {
> > > > >              match_set_tun_tos_masked(match, flower->key.tunnel.tos,
> > > >
> > > > ...
> > >
> > >
> > >
> > > --
> > > Best regards, Tonghao
> 
> 
> 
> -- 
> Best regards, Tonghao
Tonghao Zhang June 2, 2020, 1:52 p.m. UTC | #9 
On Tue, Jun 2, 2020 at 5:34 PM Simon Horman <simon.horman@netronome.com> wrote:
>
> On Tue, Jun 02, 2020 at 04:58:32PM +0800, Tonghao Zhang wrote:
> > On Tue, Jun 2, 2020 at 4:33 PM Simon Horman <simon.horman@netronome.com> wrote:
> > >
> > > On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> > > > On Fri, May 29, 2020 at 5:06 PM Simon Horman <simon.horman@netronome.com> wrote:
> > > > >
> > > > > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > > > >
> > > > > > This patch allows users to offload the TC flower rules with tunnel
> > > > > > mask. In some case, mask is useful as wildcards.
> > > > > >
> > > > > > For example:
> > > > > > $ ovs-appctl dpctl/add-flow \
> > > > > >     "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > > > > >     recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> > > > >
> > > > > Hi,
> > > > >
> > > > > Sorry for the delay in responding.
> > > > >
> > > > > I think it would be useful to spell out more clearly in the changelog
> > > > > what this patch does. From my reading of the code it:
> > > > >
> > > > > Allows masked match of the following, where previously supported
> > > > > an exact match was supported:
> > > > > * Remote (dst) tunnel endpoint address
> > > > > * Local (src) tunnel endpoint address
> > > > > * Remote (dst) tunnel endpoint UDP port
> > > > >
> > > > > And also allows masked match of the following, where previously no
> > > > > match was supported;
> > > > > * Local (std) tunnel endpoint UDP port
> > > > Ok, Thanks, I will update it in NEWS.
> > >
> > > Thanks. Please also include this information in the changelog.
> > >
> > > > > I think it would also be useful to describe a use-case where this
> > > > > is used. The command line example (above) is a good start.
> > > > Yes, I will update the commit log and describe a use-case for it.
> > > > >
> > > > > Also, not strictly related to this patch.
> > > > > I think patch series that have more than one patch should
> > > > > have a cover letter, in this case [PATCH 0/3], describing
> > > > > the overall aim of the patchset.
> > > > >
> > > > >
> > > > > The other patches in this series seem fine to me.
> > > > > Please consider addressing the issues I have raised here
> > > > > and posting a v2, with all three patches and a cover letter.
> > > > >
> > > > > ...
> > > > >
> > > > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > > > > index 875ebef71941..39cf25f63ce0 100644
> > > > > > --- a/lib/netdev-offload-tc.c
> > > > > > +++ b/lib/netdev-offload-tc.c
> > > > > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> > > > > >              match_set_tun_id(match, flower->key.tunnel.id);
> > > > > >              match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > > > > >          }
> > > > > > -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > > > > -            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > > > > > -            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > > > > > -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> > > > > > -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > > > > -            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
> > > > > > -            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
> > > > > > +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > > > > +            flower->mask.tunnel.ipv4.ipv4_src) {
> > > > >
> > > > > The change to the if condition seems separate from the change
> > > > > described in the changelog. What is the use-case for this?
> > > > I think that may be used for matching the tunnel src packets only
> > > > which will be dropped.
> > > > because user may don't want that packets sent to the host.
> > >
> > > I think it would be best to break out this (and the corresponding IPv6
> > > change) into a separate patch with a changelog that describes why
> > > this is being done and, if appropriate, an update to NEWS.
> > Hi Simon
> > Did you mean that the two patches as below as a series.
> > [PATCH 1/3] dpif-netlink: Generate ufids for installing TC flowers
> > [PATCH 3/3] netdev-offload-tc: Use ipv6_addr_is_set instead of is_all_zeros
> >
> > and this patch as a separate patch(with changelog)
> > [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
>
> Sorry for not being clear.
>
> I meant expand the series to four patch.
> Where the new patch just includes the following changes:
>
> -        if (flower->key.tunnel.ipv4.ipv4_dst) {
> +        if (flower->mask.tunnel.ipv4.ipv4_dst ||
> +            flower->mask.tunnel.ipv4.ipv4_src) {
>
> ...
>
> -        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
> -                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
>
> >
> > and other question about changelog, which file I should update, or I just update
> > it in a commit message.
>
> Sorry again for not being clear, I meant the commit message.
Hi Simon
v2 is sent, thanks for your review.
> >
> > I found the changelog in: debian/changelog
> > > Thanks!
> > >
> > > > > > +            match_set_tun_dst_masked(match,
> > > > > > +                                     flower->key.tunnel.ipv4.ipv4_dst,
> > > > > > +                                     flower->mask.tunnel.ipv4.ipv4_dst);
> > > > > > +            match_set_tun_src_masked(match,
> > > > > > +                                     flower->key.tunnel.ipv4.ipv4_src,
> > > > > > +                                     flower->mask.tunnel.ipv4.ipv4_src);
> > > > > > +        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
> > > > > > +                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
> > > > > > +            match_set_tun_ipv6_dst_masked(match,
> > > > > > +                                          &flower->key.tunnel.ipv6.ipv6_dst,
> > > > > > +                                          &flower->mask.tunnel.ipv6.ipv6_dst);
> > > > > > +            match_set_tun_ipv6_src_masked(match,
> > > > > > +                                          &flower->key.tunnel.ipv6.ipv6_src,
> > > > > > +                                          &flower->mask.tunnel.ipv6.ipv6_src);
> > > > > >          }
> > > > > >          if (flower->key.tunnel.tos) {
> > > > > >              match_set_tun_tos_masked(match, flower->key.tunnel.tos,
> > > > >
> > > > > ...
> > > >
> > > >
> > > >
> > > > --
> > > > Best regards, Tonghao
> >
> >
> >
> > --
> > Best regards, Tonghao
diff mbox series

Patch

diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
index 8af3b74ed3e0..cbb1a0382a2e 100644
--- a/include/openvswitch/match.h
+++ b/include/openvswitch/match.h
@@ -105,6 +105,8 @@  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 25c277cc670b..29b25a73bab4 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -293,6 +293,19 @@  match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
     match->flow.tunnel.tp_dst = port & 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)
 {
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 875ebef71941..39cf25f63ce0 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -633,13 +633,22 @@  parse_tc_flower_to_match(struct tc_flower *flower,
             match_set_tun_id(match, flower->key.tunnel.id);
             match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
         }
-        if (flower->key.tunnel.ipv4.ipv4_dst) {
-            match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
-            match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
-        } else if (!is_all_zeros(&flower->key.tunnel.ipv6.ipv6_dst,
-                   sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
-            match_set_tun_ipv6_src(match, &flower->key.tunnel.ipv6.ipv6_src);
-            match_set_tun_ipv6_dst(match, &flower->key.tunnel.ipv6.ipv6_dst);
+        if (flower->mask.tunnel.ipv4.ipv4_dst ||
+            flower->mask.tunnel.ipv4.ipv4_src) {
+            match_set_tun_dst_masked(match,
+                                     flower->key.tunnel.ipv4.ipv4_dst,
+                                     flower->mask.tunnel.ipv4.ipv4_dst);
+            match_set_tun_src_masked(match,
+                                     flower->key.tunnel.ipv4.ipv4_src,
+                                     flower->mask.tunnel.ipv4.ipv4_src);
+        } else if (ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_dst) ||
+                   ipv6_addr_is_set(&flower->mask.tunnel.ipv6.ipv6_src)) {
+            match_set_tun_ipv6_dst_masked(match,
+                                          &flower->key.tunnel.ipv6.ipv6_dst,
+                                          &flower->mask.tunnel.ipv6.ipv6_dst);
+            match_set_tun_ipv6_src_masked(match,
+                                          &flower->key.tunnel.ipv6.ipv6_src,
+                                          &flower->mask.tunnel.ipv6.ipv6_src);
         }
         if (flower->key.tunnel.tos) {
             match_set_tun_tos_masked(match, flower->key.tunnel.tos,
@@ -649,8 +658,15 @@  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->key.tunnel.tp_dst) {
-            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
+        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.metadata.present.len) {
             flower_tun_opt_to_match(match, flower);
@@ -1404,8 +1420,14 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
         flower.key.tunnel.ttl = tnl->ip_ttl;
         flower.key.tunnel.tp_src = tnl->tp_src;
         flower.key.tunnel.tp_dst = tnl->tp_dst;
+        flower.mask.tunnel.ipv4.ipv4_src = tnl_mask->ip_src;
+        flower.mask.tunnel.ipv4.ipv4_dst = tnl_mask->ip_dst;
+        flower.mask.tunnel.ipv6.ipv6_src = tnl_mask->ipv6_src;
+        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 12af0192b614..9ac40f692c17 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -372,6 +372,12 @@  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,
@@ -650,22 +656,38 @@  nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
         flower->mask.tunnel.id = OVS_BE64_MAX;
     }
     if (attrs[TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK]) {
+        flower->mask.tunnel.ipv4.ipv4_src =
+            nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK]);
         flower->key.tunnel.ipv4.ipv4_src =
             nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_SRC]);
     }
     if (attrs[TCA_FLOWER_KEY_ENC_IPV4_DST_MASK]) {
+        flower->mask.tunnel.ipv4.ipv4_dst =
+            nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_DST_MASK]);
         flower->key.tunnel.ipv4.ipv4_dst =
             nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_IPV4_DST]);
     }
     if (attrs[TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK]) {
+        flower->mask.tunnel.ipv6.ipv6_src =
+            nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK]);
         flower->key.tunnel.ipv6.ipv6_src =
             nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_SRC]);
     }
     if (attrs[TCA_FLOWER_KEY_ENC_IPV6_DST_MASK]) {
+        flower->mask.tunnel.ipv6.ipv6_dst =
+            nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST_MASK]);
         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_DST_PORT]) {
+    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]);
         flower->key.tunnel.tp_dst =
             nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
     }
@@ -2594,11 +2616,18 @@  nl_msg_put_flower_tunnel_opts(struct ofpbuf *request, uint16_t type,
 static void
 nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
 {
+    ovs_be32 ipv4_src_mask = flower->mask.tunnel.ipv4.ipv4_src;
+    ovs_be32 ipv4_dst_mask = flower->mask.tunnel.ipv4.ipv4_dst;
     ovs_be32 ipv4_src = flower->key.tunnel.ipv4.ipv4_src;
     ovs_be32 ipv4_dst = flower->key.tunnel.ipv4.ipv4_dst;
+    struct in6_addr *ipv6_src_mask = &flower->mask.tunnel.ipv6.ipv6_src;
+    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;
@@ -2606,12 +2635,21 @@  nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
     uint8_t ttl_mask = flower->mask.tunnel.ttl;
     ovs_be64 id_mask = flower->mask.tunnel.id;
 
-    if (ipv4_dst) {
-        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_SRC, ipv4_src);
+    if (ipv4_dst_mask || ipv4_src_mask) {
+        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_DST_MASK,
+                        ipv4_dst_mask);
+        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK,
+                        ipv4_src_mask);
         nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_DST, ipv4_dst);
-    } else if (!is_all_zeros(ipv6_dst, sizeof *ipv6_dst)) {
-        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_SRC, ipv6_src);
+        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_SRC, ipv4_src);
+    } else if (ipv6_addr_is_set(ipv6_dst_mask) ||
+               ipv6_addr_is_set(ipv6_src_mask)) {
+        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_DST_MASK,
+                            ipv6_dst_mask);
+        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK,
+                            ipv6_src_mask);
         nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_DST, ipv6_dst);
+        nl_msg_put_in6_addr(request, TCA_FLOWER_KEY_ENC_IPV6_SRC, ipv6_src);
     }
     if (tos_mask) {
         nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TOS, tos);
@@ -2621,9 +2659,16 @@  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) {
+    if (tp_dst_mask) {
+        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
+                        tp_dst_mask);
         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 d65bf4412aa9..d3fdbbe3c4d3 100644
--- a/tests/tunnel.at
+++ b/tests/tunnel.at
@@ -110,6 +110,28 @@  Datapath actions: drop
 OVS_VSWITCHD_STOP(["/dropping tunnel packet marked ECN CE but is not ECN capable/d"])
 AT_CLEANUP
 
+AT_SETUP([tunnel - input with matching tunnel mask])
+OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
+                    options:remote_ip=1.1.1.1 \
+                    ofport_request=1	\
+                    -- add-port br0 p2 -- set Interface p2 type=dummy \
+                    ofport_request=2])
+
+AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
+    br0 65534/100: (dummy-internal)
+    p1 1/1: (gre: remote_ip=1.1.1.1)
+    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/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
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([tunnel - output])
 OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
                     options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \