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 |
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
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>
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, ...
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!
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, > > ...
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
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
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
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 --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 \