On Wed, Feb 23, 2022 at 6:21 PM Numan Siddique <[email protected]> wrote: > > On Wed, Feb 23, 2022 at 7:57 PM Han Zhou <[email protected]> wrote: > > > > On Wed, Feb 23, 2022 at 2:51 PM Numan Siddique <[email protected]> wrote: > > > > > > On Tue, Feb 22, 2022 at 10:18 AM Numan Siddique <[email protected]> wrote: > > > > > > > > On Thu, Feb 17, 2022 at 5:54 PM Han Zhou <[email protected]> wrote: > > > > > > > > > > On Thu, Feb 17, 2022 at 1:18 PM Mark Michelson < [email protected]> > > wrote: > > > > > > > > > > > > Hi Han, > > > > > > > > > > > > I had a look through this series and to be honest, I probably don't > > have > > > > > > the expertise to critique every line of code. However, the test > > cases > > > > > > you added, plus my own independent "make sure nothing falls apart" > > > > > > tests, give me enough confidence to OK this patch series. > > > > > > > > > > > > Acked-by: Mark Michelson <[email protected]> > > > > > > > > > > > Thanks Mark for the review! Since Numan mentioned that he will review > > it as > > > > > well, I will wait for his feedback. It is always better to have more > > eyes > > > > > on the patches. > > > > > > > > Thanks. I've started reviewing the patches and hoping to provide the > > > > comments soon. > > > > > > Thanks for working on this series and improving the performance. > > > > > > For the entire series: Acked-by: Numan Siddique <[email protected]> > > > > > > I have left a few small comments for 2 patches. Please check them out > > > before applying. > > > > > > > Thanks Numan and Mark for the review! I applied the series to main with the > > comments addressed. > > Hi Han, > > clang tests are failing due to memory leaks in the github CI and > locally as well. > > I apologize. I should have checked this earlier. > It is my fault :( I think I forgot to run ASAN again after fixing things, and I am really sorry about that. I sent two fixes:
https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ PTAL. Thanks, Han > Can you please take a look - > https://github.com/ovn-org/ovn/runs/5312982212?check_suite_focus=true > > ---- > ================================================================= > ==3988733==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 56 byte(s) in 7 object(s) allocated from: > #0 0x7fd30e4bf91f in __interceptor_malloc (/lib64/libasan.so.6+0xae91f) > #1 0x5ec75d in xmalloc__ ../lib/util.c:137 > #2 0x5ec75d in xmalloc ../lib/util.c:172 > #3 0x4d8e89 in parse_constant ../lib/expr.c:932 > #4 0x4d91c2 in parse_constant_set ../lib/expr.c:971 > #5 0x4dc853 in expr_parse_primary ../lib/expr.c:1390 > #6 0x4dd2cd in expr_parse_not ../lib/expr.c:1479 > #7 0x4dd32b in expr_parse__ ../lib/expr.c:1486 > #8 0x4dd840 in expr_parse ../lib/expr.c:1535 > #9 0x4dd9c8 in expr_parse_string ../lib/expr.c:1557 > #10 0x42c3b7 in convert_match_to_expr ../controller/lflow.c:1240 > #11 0x42e09d in consider_logical_flow__ ../controller/lflow.c:1407 > #12 0x42eedf in consider_logical_flow ../controller/lflow.c:1583 > #13 0x425725 in lflow_handle_changed_flows ../controller/lflow.c:483 > #14 0x492029 in lflow_output_sb_logical_flow_handler > ../controller/ovn-controller.c:2462 > #15 0x4fac2e in engine_compute ../lib/inc-proc-eng.c:406 > #16 0x4fb233 in engine_run_node ../lib/inc-proc-eng.c:468 > #17 0x4fb2e7 in engine_run ../lib/inc-proc-eng.c:493 > #18 0x49ad62 in main ../controller/ovn-controller.c:3636 > #19 0x7fd30dc6f55f in __libc_start_call_main (/lib64/libc.so.6+0x2d55f) > > SUMMARY: AddressSanitizer: 56 byte(s) leaked in 7 allocation(s). > ../../tests/ovs-macros.at:230: hard failure > 548. ovn.at:24011: 548. Big Load Balancer -- ovn-northd -- > dp-groups=no (ovn.at:24011): FAILED (ovs-macros.at:230) > > ----- > > Thanks > Numan > > > > > Han > > > > > Numan > > > > > > > > > > > Numan > > > > > > > > > > > > > > Thanks, > > > > > Han > > > > > > > > > > > On 2/9/22 12:54, Han Zhou wrote: > > > > > > > Today although the incremental processing engine of ovn-controller > > > > > handles > > > > > > > address set changes incrementally, it is at the logical flow level > > > > > instead of > > > > > > > individual addresses level. A single address change in an address > > set > > > > > would > > > > > > > cause all the related logical flows being reprocessed. The cost > > of > > > > > > > reprocessing a lflow referencing a big address set can be very > > high. > > > > > When the > > > > > > > change rate of anaddress sets is high, ovn-controller would be > > busy > > > > > reprocessing > > > > > > > logical flows. > > > > > > > > > > > > > > This patch series optimizes this typical scenario for large scale > > > > > environment > > > > > > > by incrementally processing each individual address updates. When > > the > > > > > change is > > > > > > > small (e.g. adding/deleting a single address in an address set), > > this > > > > > results > > > > > > > in constant processing time, regardless of the size of the > > address set. > > > > > > > > > > > > > > There are limitations that these approaches can't apply. For > > example, > > > > > when an > > > > > > > ACL is in the below forms: > > > > > > > > > > > > > > ip.src == $as1 || ip.dst == $as2 > > > > > > > ip.src == {$as1, $as2} > > > > > > > ip.src == {$as1, ip1} > > > > > > > > > > > > > > In these cases during lflow parsing the expressions are combined > > to a > > > > > single > > > > > > > OR, which loses the tracking information for the address sets' > > IPs and > > > > > flows > > > > > > > generated. There are other cases that can't be handled that are > > > > > documented for > > > > > > > the function lflow_handle_addr_set_update, and also added in test > > > > > cases. In > > > > > > > these cases it just fall back to the old approach that > > reprocesses the > > > > > > > lflow. So, this change doesn't add any new constraint to the > > users, but > > > > > just > > > > > > > leave some cases as unoptimized as it was before. > > > > > > > > > > > > > > Scale test shows obvious performance gains because the time > > complexity > > > > > > > changed from O(n) to O(1). The bigger the size of address set, > > the more > > > > > > > CPU savings. With the AS size of 10k, the test shows ~40x speed > > up. > > > > > > > > > > > > > > Test setup: > > > > > > > CPU: Intel(R) Core(TM) i9-7920X CPU @ 2.90GHz. > > > > > > > 5 ACL all referencing an address set of 10,000 IPs. > > > > > > > > > > > > > > Measure the time spent by ovn-controller for handling one IP > > deletion > > > > > > > from the address set: > > > > > > > > > > > > > > Before: ~400ms > > > > > > > After: 11-12ms > > > > > > > > > > > > > > There is memory cost increase, due to the index built to track > > each > > > > > individual > > > > > > > addresses. The total memory cost for the OF flows in > > ovn-controller > > > > > increased > > > > > > > ~20% in the 10k AS size test. > > > > > > > > > > > > > > Before: > > > > > > > ofctrl_desired_flow_usage-KB:22248 > > > > > > > ofctrl_installed_flow_usage-KB:14850 > > > > > > > ofctrl_sb_flow_ref_usage-KB:7208 > > > > > > > > > > > > > > After: > > > > > > > ofctrl_desired_flow_usage-KB:22248 > > > > > > > ofctrl_installed_flow_usage-KB:14850 > > > > > > > ofctrl_sb_flow_ref_usage-KB:15551 > > > > > > > > > > > > > > --- > > > > > > > v1 -> v2: > > > > > > > - Fixed a build error of patch 4, which was caused by misplacing a > > > > > change to > > > > > > > patch 4 which should have been in patch 5. Updated patch 5 > > commit > > > > > message as > > > > > > > well. > > > > > > > > > > > > > > Changes after RFC: > > > > > > > - Added a new patch for maintaining ref_count in logical flow > > resource > > > > > > > reference for resource type: address-set, which is needed for > > the > > > > > correctness > > > > > > > of both IP addition and deletion I-P in some corner cases. > > > > > > > - Fixed the corner case when the same address set is used multiple > > > > > times in the > > > > > > > same lflow with one of the references untrackable. Added test > > case > > > > > to cover > > > > > > > as well. > > > > > > > - Added more documentation, such as the limitations and corner > > cases. > > > > > > > - Added tests for ipv6 and mac address support. > > > > > > > - Other minor improvements. > > > > > > > > > > > > > > Han Zhou (11): > > > > > > > expr.c: Use expr_destroy and expr_clone instead of free and > > xmemdup. > > > > > > > ofctrl.c: Combine remove_flows_from_sb_to_flow and > > > > > > > ofctrl_flood_remove_flows. > > > > > > > ovn-controller: Track individual IP information of address set > > during > > > > > > > lflow parsing. > > > > > > > ovn-controller.c: Remove unnecessary asserts and useless > > variables. > > > > > > > ovn-controller.c: Refactor init_lflow_ctx. > > > > > > > ovn-controller: Tracking SB address set updates. > > > > > > > lflow.c: Set "changed" properly in lflow_handle_changed_ref(). > > > > > > > ovn-controller: Add tests for different ACL address set usage > > > > > > > patterns. > > > > > > > lflow: Track reference count of address sets when parsing > > lflows. > > > > > > > ovn-controller: Handle addresses deletion in address set > > > > > > > incrementally. > > > > > > > ovn-controller: Handle addresses addition in address set > > > > > > > incrementally. > > > > > > > > > > > > > > controller/lflow-conj-ids.c | 20 + > > > > > > > controller/lflow-conj-ids.h | 1 + > > > > > > > controller/lflow.c | 532 +++++++++++++++- > > > > > > > controller/lflow.h | 13 + > > > > > > > controller/ofctrl.c | 329 +++++++--- > > > > > > > controller/ofctrl.h | 24 +- > > > > > > > controller/ovn-controller.c | 278 ++++---- > > > > > > > controller/physical.c | 2 +- > > > > > > > include/ovn/expr.h | 24 +- > > > > > > > lib/expr.c | 274 ++++++-- > > > > > > > tests/ovn-controller.at | 1186 > > +++++++++++++++++++++++++++++++++++ > > > > > > > 11 files changed, 2378 insertions(+), 305 deletions(-) > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > 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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
