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

[Simon Horman]

On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m.yue at gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue at 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,

...