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.
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
