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
