On Mon, Aug 12, 2024 at 9:10 AM Ilya Maximets <[email protected]> wrote: > > On 8/12/24 06:16, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst > > field. These flows when translated to datapath flows also match on > > ip6.dst, which means a separate datapath flow per destination IP > > address. This may cause significant performance issues in some > > setups (particularly ovs-dpdk telco deployments). > > > > This patch addresses this issue by matching on eth.mcast6 so that > > datapath flows for normal IPv6 traffic doesn't have to match on > > ip6.dst. IPv6 NS packets are generally multicast. A new logical > > match "nd_ns_mcast" for this purpose. > > > > After this patch, We no longer respond to IPv6 NS unicast packets. > > This behavior now matches the IPv4 ARP responder flows. Note > > that after the commit [1] which was recently added we now only > > respond to IPv4 ARP broadcast packets. > > Hi, Numan. Since we sorted out the question I had in the other thread, > here is some code review (I didn't actually review the test). > > It would be great if you include some of the rationale for the change > from commit [1] right in here, so it's easier to understand the whole > picture without jumping between commits. > > > > > A recent patch [2] from Ilya partially addressed the same datapath > > flow explosion issue by matching on eth.mcast6 for MLD packets. > > With this patch, we now completely address the datapath flow > > explosion issue for IPv6 traffic provided all the logical ports > > of a logical switch are not configured with port security. > > > > [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") > > [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD > > and IGMP.") > > > > Reported-at: https://issues.redhat.com/browse/FDP-728 > > Reported-by: Mike Pattrick <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > lib/logical-fields.c | 3 ++ > > northd/northd.c | 11 ++-- > > tests/ovn-northd.at | 4 +- > > tests/ovn.at | 2 +- > > tests/system-ovn.at | 126 +++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 138 insertions(+), 8 deletions(-) > > > > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > > index 2c9d3c61bf..f1ba1a7ac1 100644 > > --- a/lib/logical-fields.c > > +++ b/lib/logical-fields.c > > @@ -293,6 +293,9 @@ ovn_init_symtab(struct shash *symtab) > > "icmp6.type == {135, 136} && icmp6.code == 0 && ip.ttl == > > 255"); > > expr_symtab_add_predicate(symtab, "nd_ns", > > "icmp6.type == 135 && icmp6.code == 0 && ip.ttl == 255"); > > + expr_symtab_add_predicate(symtab, "nd_ns_mcast", > > + "eth.mcastv6 && icmp6.type == 135 && icmp6.code == 0 && " > > Should this be an 'ip6.mcast' instead? It should be more correct this way. > The expression parser will remove redundancy, if any. > > > + "ip.ttl == 255"); > > expr_symtab_add_predicate(symtab, "nd_na", > > "icmp6.type == 136 && icmp6.code == 0 && ip.ttl == 255"); > > expr_symtab_add_predicate(symtab, "nd_rs", > > diff --git a/northd/northd.c b/northd/northd.c > > index 5ad30d854b..c0cda85e36 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -9638,11 +9638,12 @@ build_lswitch_arp_nd_responder_known_ips(struct > > ovn_port *op, > > * but always respond with the unicast IPv6 address. */ > > The comment above is talking about matching unicast traffic, which is > no longer the case. > > > for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) { > > ds_clear(match); > > - ds_put_format(match, > > - "nd_ns && ip6.dst == {%s, %s} && nd.target == %s", > > - op->lsp_addrs[i].ipv6_addrs[j].addr_s, > > - op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, > > - op->lsp_addrs[i].ipv6_addrs[j].addr_s); > > + ds_put_format( > > + match, > > + "nd_ns_mcast && ip6.dst == {%s, %s} && nd.target == > > %s", > > + op->lsp_addrs[i].ipv6_addrs[j].addr_s, > > The combination of 'nd_ns_mcast' and the unicast 'addr_s' is incorrect, AFAIU. > Should we only match on 'sn_addr_s' here? > > > + op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, > > + op->lsp_addrs[i].ipv6_addrs[j].addr_s); > > > > ds_clear(actions); > > ds_put_format(actions, > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index e4c8822656..509cfd2d30 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -9490,9 +9490,9 @@ AT_CAPTURE_FILE([S1flows]) > > AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_arp_rsp ), priority=0 , match=(1), action=(next;) > > table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa == > > 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff && inport == > > "S1-vm1"), action=(next;) > > - table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns && ip6.dst > > == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == > > "S1-vm1"), action=(next;) > > + table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns_mcast && > > ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport > > == "S1-vm1"), action=(next;) > > table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == > > 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff), > > action=(eth.dst = eth.src; eth.src = 50:54:00:00:00:10; arp.op = 2; /* ARP > > reply */ arp.tha = arp.sha; arp.sha = 50:54:00:00:00:10; arp.tpa = arp.spa; > > arp.spa = 192.168.0.10; outport = inport; flags.loopback = 1; output;) > > - table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst > > == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { > > eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; > > nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; > > };) > > + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns_mcast && > > ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), > > action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target > > = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = > > 1; output; };) > > ]) > > > > #Set the disable_arp_nd_rsp option and verify the flow > > diff --git a/tests/ovn.at b/tests/ovn.at > > index a1d689e849..96a89c7d8c 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -14417,7 +14417,7 @@ test_ns_na() { > > local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 > > > > packet=$(fmt_pkt " > > - Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}') / > > + Ether(dst='33:33:ff:ff:ff:ff', src='${src_mac}') / > > IPv6(src='${src_ip}', dst='ff02::1:ff00:2') / > > ICMPv6ND_NS(tgt='${dst_ip}') > > ") > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index c54b0f3a5f..980472f4e0 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -13750,3 +13750,129 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > /.*terminating with signal 15.*/d"]) > > AT_CLEANUP > > ]) > > + > > +dnl This test sends IPv6 TCP packets between two lports of the same > > +dnl logical switch and checks that the datapath flows don't match on > > +dnl IPv6 source and destination addresses. > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- IPv6 switching - datapath flows]) > > +AT_KEYWORDS([IPv6]) > > + > > +ovn_start > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-int]) > > + > > +dnl Set external-ids in br-int needed for ovn-controller > > +check ovs-vsctl \ > > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > > + -- set Open_vSwitch . > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > + -- set bridge br-int fail-mode=secure > > other-config:disable-in-band=true > > + > > +dnl Start ovn-controller > > +start_daemon ovn-controller > > + > > +check ovn-nbctl ls-add sw0 > > + > > +check ovn-nbctl lsp-add sw0 vm0 > > +check ovn-nbctl lsp-set-addresses vm0 "f0:00:0f:01:02:03 10.0.0.3 1000::3" > > + > > +check ovn-nbctl lsp-add sw0 vm1 > > +check ovn-nbctl lsp-set-addresses vm1 "f0:00:0f:01:02:04 10.0.0.4 1000::4" > > + > > +check ovn-nbctl --wait=hv sync > > + > > +ovn-sbctl dump-flows sw0 > /tmp/c.txt > > + > > +ADD_NAMESPACES(vm0) > > +ADD_VETH(vm0, vm0, br-int, "1000::3/64", "f0:00:0f:01:02:03", "1000::1", > > "nodad") > > + > > +ADD_NAMESPACES(vm1) > > +ADD_VETH(vm1, vm1, br-int, "1000::4/64", "f0:00:0f:01:02:04", "1000::1", > > "nodad") > > + > > +dnl Checks the datapath flows and makes sure that there are no matches to > > +dnl IPv6 src or dst fields. > > +check_dp_flows() { > > + ipv6_src_dst_match=$1 > > + dnl Dump the flows. Will be helpful while debugging. > > + ovs-appctl dpctl/dump-flows > > + > > + eth_match1="eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04)" > > + eth_match2="eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03)" > > + ipv6_src_match="ipv6(src=" > > + ipv6_dst_match="ipv6(dst=" > > + > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" -c], [0], [dnl > > +2 > > +]) > > + > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" -c], [0], [dnl > > +2 > > +]) > > + > > + if [[ "$ipv6_src_dst_match" == "yes" ]]; then > > + dnl Check that the dp flow for > > eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) > > + dnl matches on IPv6 src or dst. > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep -e > > "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl > > +2 > > +]) > > + > > + dnl Check that the dp flow for > > eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) > > + dnl matches on IPv6 src or dst. > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep -e > > "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl > > +2 > > +]) > > + else > > + dnl Check that the dp flow for > > eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) > > + dnl doesn't match on IPv6 src or dst. > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep > > "$ipv6_src_match"], [1], []) > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep > > "$ipv6_dst_match"], [1], []) > > + > > + dnl Check that the dp flow for > > eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep > > "$ipv6_src_match"], [1], []) > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep > > "$ipv6_dst_match"], [1], []) > > + fi > > +} > > + > > +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ > > +[0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > +# Start IPv6 TCP server on vm0. > > +NETNS_DAEMONIZE([vm0], [nc -6 -l -k 1000::3 4242], [nc-vm1-1.pid]) > > +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) > > + > > +check_dp_flows no > > + > > +# Now configure port security on vm0. dp flows should now match on IPv6 > > fields. > > +check ovn-nbctl lsp-set-port-security vm0 "f0:00:0f:01:02:03 10.0.0.3 > > 1000::3" > > +check ovn-nbctl --wait=hv sync > > + > > +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ > > +[0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > +# ovs-appctl dpctl/del-flows || : > > Leftover?
Thanks for the review comments. I've addressed all of them v2 is submitted here - https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Numan > > > +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) > > +check_dp_flows yes > > + > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > + > > +as ovn-sb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as ovn-nb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as northd > > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > +/failed to query port patch-.*/d > > +/.*terminating with signal 15.*/d"]) > > +AT_CLEANUP > > +]) > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
