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.hor...@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.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. > > > > 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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev