On Wed, Aug 9, 2023 at 9:38 AM <[email protected]> wrote: > > From: Numan Siddique <[email protected]> > Thanks Numan for the enhancement.
> Instead of maintaing the lport to lflow references using > 'struct lflow_ref_list', this patch makes use of the existing > objdep APIs. Since objdep_mgr is not thread safe, an instance > of objdep_mgr is maintained for each 'ovn_port'. > > Once we add the thread safe support to objdep APIs we can move > the objdep_mgr to 'lflow' engine date. > > Using objdep APIs does come with a cost in memory. We now > maintain an additional hmap of ovn_lflow's to look up using > uuid. But this will be useful to handle datapath and load > balancer changes in the lflow engine node. > I understand that the 'struct lflow_ref_list' may not satisfy the needs of more complex I-P, but I wonder if objdep_mgr (without modification) is the solution. It seems to me more complex at least for this patch without adding obvious benefits. Since I am still reviewing, maybe I missed some points which would show the value from the rest of the patches, and please forgive me for my slow review. While I am still reviewing the patches, I did a performance test with https://github.com/hzhou8/ovn-test-script for the 200 chassis x 50 lsp scale of simulated ovn-k8s topology. - Before this patch, a recompute takes 1293ms - After this patch, a recompute takes 1536ms, which is 20% more time. - I also did the same test with all the rest of the patches of this series, which takes 1690ms. So the major increase of the recompute time is from this patch. I didn't debug what's the exact cause of this increase, but I think it might be related to the objdep usage. > This patch does few more changes which are significant: > - Logical flows are now hashed without the logical > datapaths. If a logical flow is referenced by just one > datapath, we don't rehash it. > > - The synthetic 'hash' column of sbrec_logical_flow now > doesn't use the logical datapath too. This means that > when ovn-northd is updated/upgraded and has this commit, > all the logical flows with 'logical_datapath' column > set will get deleted and re-added causing some disruptions. > > - With the commit [1] which added I-P support for logical > port changes, multiple logical flows with same match 'M' > and actions 'A' are generated and stored without the > dp groups, which was not the case prior to > that patch. > One example to generate these lflows is: > ovn-nbctl lsp-set-addresses sw0p1 "MAC1 IP1" > ovn-nbctl lsp-set-addresses sw1p1 "MAC1 IP1" > ovn-nbctl lsp-set-addresses sw2p1 "MAC1 IP1" > > Now with this patch we go back to the earlier way. i.e > one logical flow with logical_dp_groups set. > Thanks for taking the effort to achieve this. The approach is indeed very smart. I was aware of such scenarios, but it just didn''t seem to impact scalability because the number of such lflows should be small, so it wasn't considered a priority. > - With this patch any updates to a logical port which > doesn't result in new logical flows will not result in > deletion and addition of same logical flows. > Eg. > ovn-nbctl set logical_switch_port sw0p1 external_ids:foo=bar > will be a no-op to the SB logical flow table. > Similar to the above case, this doesn't seem to be critical for scalability, but it is good to see the enhancement. I will post more feedback after finishing the full review. Thanks, Han > [1] - 8bbd67891f68("northd: Incremental processing of VIF additions in 'lflow' node.") > > Suggested-by: Dumitru Ceara <[email protected]> > Signed-off-by: Numan Siddique <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
