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

Simon Horman simon.horman at netronome.com
Tue Jun 2 09:34:25 UTC 2020


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 at 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 at netronome.com> wrote:
> > > >
> > > > 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
> > > 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


More information about the dev mailing list