On Fri, Aug 16, 2024 at 8:13 AM Ilya Maximets <i.maxim...@ovn.org> wrote: > > On 8/15/24 18:52, num...@ovn.org wrote: > > From: Numan Siddique <num...@ovn.org> > > > > IPv6 ND 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" is added for this purpose. > > > > After this patch, We no longer respond to IPv6 NS unicast packets. > > Let the target reply to it, so that the sender has the ability to > > monitor the targe liveness via the unicast ND solicitations. > > 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. > > > > 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 address the datapath flow explosion issue > > for IPv6 traffic provided 2 conditions are met: > > a. All the logical ports of a logical switch are not configured > > with port security. > > b. The logical switch port of type router if configured > > with "arp_proxy" option doesn't include any IPv6 address(es). > > > > [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") > > [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD > > and IGMP.") > > > > Note: Documentation for 'eth.mcastv6' and 'ip6.mcast' predicates were > > missing from ovn-sb.xml and this patch adds it. > > > > Reported-at: https://issues.redhat.com/browse/FDP-728 > > Reported-by: Mike Pattrick <m...@redhat.com> > > Signed-off-by: Numan Siddique <num...@ovn.org> > > --- > > > > v2 -> v3 > > ------- > > - Removed the system test as it was flaky and intead added the test in > > ovn.at and used ofproto/trace as suggested by Ilya to check the > > megaflows. > > > > v1 -> v2 > > ------- > > - Addressed review comments from Ilya. > > > > > > lib/logical-fields.c | 3 + > > northd/northd.c | 21 ++++--- > > ovn-sb.xml | 3 + > > tests/ovn-northd.at | 4 +- > > tests/ovn.at | 144 ++++++++++++++++++++++++++++++++++++++++++- > > 5 files changed, 163 insertions(+), 12 deletions(-) > > Thanks, Numan! I didn't review the test, but the code looks good > to me. There is a couple of typos in the comment, see below. > > For the code with typos fixed: > > Acked-by: Ilya Maximets <i.maxim...@ovn.org> > > > > > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > > index 2c9d3c61bf..5a8b53f2b6 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", > > + "ip6.mcast && icmp6.type == 135 && icmp6.code == 0 && " > > + "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 c4824aae3b..c9ae1e958f 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -9633,16 +9633,21 @@ build_lswitch_arp_nd_responder_known_ips(struct > > ovn_port *op, > > op->lflow_ref); > > } > > > > - /* For ND solicitations, we need to listen for both the > > - * unicast IPv6 address and its all-nodes multicast address, > > - * but always respond with the unicast IPv6 address. */ > > + /* For ND solicitations: > > + * - Reply only for the all-nodes multicast address(es) of > > the > > + * logical port IPv6 address(es). > > + * > > + * - Do not reply for unicast ND solicitations. Let the > > target > > + * reply it, so that the sender has the ability to monitor > > "reply to it" ? > > > + * the targe liveness via the unicast ND solicitations. > > *target > > > + */ > > 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 && nd.target == %s", > > + 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, > > Best regards, Ilya Maximets.
Thanks Dumitru and Ilya for the reviews. I applied this patch to the main and branch-24.09 after addressing the review comments. After the CI passes for other branches in my private github CI, I'll backport to other branches. Numan > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev