On Fri, May 29, 2020 at 5:06 PM Simon Horman <simon.hor...@netronome.com> wrote:
>
> On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m....@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m....@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,
>
> ...
--
Best regards, Tonghao
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
