On Fri, Mar 14, 2025 at 12:35 AM Numan Siddique <num...@ovn.org> wrote: > > On Thu, Mar 13, 2025 at 12:21 PM Han Zhou <hz...@ovn.org> wrote: > > > > > > > > On Sat, Mar 1, 2025 at 6:38 AM <num...@ovn.org> wrote: > >> > >> From: Numan Siddique <num...@ovn.org> > >> > >> This patch series optimizes ovn-controller to add only relevant > >> datapaths to its "local datapaths" map if the logical topology > >> has a single flat provider logical switch connected to multiple > >> routers with a distributed gateway port. > >> > >> (Patch 3 has a detailed commit message.) > >> > >> v1 -> v2 > >> ----- > >> * Rebased to resolve the conflicts. > >> > >> Numan Siddique (3): > >> controller: Store local binding lports in local_datapath. > >> northd: Add a flag 'only_dgp_peer_ports' to the SB Datapath. > >> controller: Optimize adding 'dps' to the local datapaths. > >> > >> controller/binding.c | 324 +++++++++++--- > >> controller/binding.h | 2 + > >> controller/local_data.c | 98 ++++- > >> controller/local_data.h | 10 +- > >> controller/lport.c | 12 + > >> controller/lport.h | 4 + > >> controller/ovn-controller.c | 38 ++ > >> northd/northd.c | 31 +- > >> northd/northd.h | 4 + > >> tests/multinode.at | 185 +++++++- > >> tests/ovn-northd.at | 59 +++ > >> tests/ovn-performance.at | 6 +- > >> tests/ovn.at | 853 ++++++++++++++++++++++++++++++++++++ > >> 13 files changed, 1545 insertions(+), 81 deletions(-) > >> > >> -- > >> 2.48.1 > >> > >> _______________________________________________ > >> dev mailing list > >> d...@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > Hi Numan, > > > > Sorry for the slow review. I have a general comment for this series. I believe the exact same performance optimization is supposed to be achieved by the below commit: > > > > 22298fd37 ovn-controller: Don't flood fill local datapaths beyond DGP boundary. > > > > The scenarios are mentioned in detail in the commit message of that patch, which is very similar to what you described here. > > > > The only exception was when there are distributed NAT or when the redirection type is set to "bridged". In these cases it will fall back to add peer datapath as local. See SB:Port_Binding:options:always-redirect. Is this something that your patch is targeting? Otherwise I wonder why it didn't work without your patch. > > Hi Han, > > I wanted to mention in the commit message and in the cover letter that > this patch series is an extension of the commit 22298fd37. I somehow > missed it. > > Yes, I'm targeting the scenario of distributed NAT, >
I'd request to update the commit message and focus on how the patch addresses the distributed NAT scenario. Otherwise it would be misleading, since the problem doesn't exist for the scenario described in the current commit message. In addition, could you explain a little more about the solution. How does the has_only_dgp_peer_ports address the distributed NAT scenario? For my understanding, when there is distributed NAT associated to the LR, the chassis needs to know the provider LS so that the NAT pipeline can work on the chassis. If the peer LS (provider LS) is not added just because of has_only_dgp_peer_ports, how would the distributed NAT work? I didn't see any handling in your change related to distributed NAT. I believe I missed something, because I saw your test case purposely used two LRs, one with distributed NAT and one without, and the one with distributed NAT did have provider LS added to the chassis, although I didn't find how that worked. It would be helpful for code review if you could highlight the related logic, probably also helpful for maintaining the code by adding related comments in the code for this specific consideration. On the other hand, I also have doubts about the criteria of deciding not to add peers according to has_only_dgp_peer_ports. When the peer is a provider LS and connected by DGP, we shouldn't add the peer, regardless of whether there is another LR connected to the provider LS with a distributed LRP, right? The provider LS (and the other LR, say, LR-X) should be added to the chassis if there is a port-binding on the chassis that is connected (directly or indirectly) to that LR-X, right? But before this, I think I need to understand firstly how the distributed NAT scenario is addressed by your patch. Thanks, Han > Numan > > > > Could you help clarify? > > > > Thanks, > > Han > > > >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
