| Message ID | 1534182499-75914-1-git-send-email-hzhou8@ebay.com |
|---|---|
| Headers | show |
| Series | ovn-controller incremental processing | expand |
On Mon, Aug 13, 2018 at 10:47:59AM -0700, Han Zhou wrote: > ovn-controller currently recomputes everything when there are any changes > of input, which leads to high CPU usages and slow in end-to-end flow > enforcement in response to changes. It even wastes CPU to recompute flows > for unrelated inputs such as pinctrl events. Would you mind publishing this as a branch I can pull from?
On Wed, Sep 5, 2018 at 2:34 PM Ben Pfaff <blp@ovn.org> wrote: > > On Mon, Aug 13, 2018 at 10:47:59AM -0700, Han Zhou wrote: > > ovn-controller currently recomputes everything when there are any changes > > of input, which leads to high CPU usages and slow in end-to-end flow > > enforcement in response to changes. It even wastes CPU to recompute flows > > for unrelated inputs such as pinctrl events. > > Would you mind publishing this as a branch I can pull from? Hi Ben, The branch for the series is here: https://github.com/hzhou8/ovs/tree/ip12 Thanks, Han
On Wed, Sep 05, 2018 at 05:11:01PM -0700, Han Zhou wrote: > On Wed, Sep 5, 2018 at 2:34 PM Ben Pfaff <blp@ovn.org> wrote: > > > > On Mon, Aug 13, 2018 at 10:47:59AM -0700, Han Zhou wrote: > > > ovn-controller currently recomputes everything when there are any > changes > > > of input, which leads to high CPU usages and slow in end-to-end flow > > > enforcement in response to changes. It even wastes CPU to recompute > flows > > > for unrelated inputs such as pinctrl events. > > > > Would you mind publishing this as a branch I can pull from? > > Hi Ben, > > The branch for the series is here: https://github.com/hzhou8/ovs/tree/ip12 Thanks. I am looking at it. I built the tip of the branch and saw only two warnings, from sparse: ../ovn/controller/ovn-controller.c:616:12: error: symbol 'sb_engine_node_names' was not declared. Should it be static? ../ovn/controller/ovn-controller.c:641:12: error: symbol 'ovs_engine_node_names' was not declared. Should it be static? Usually this warning means that the function or variable should be marked static.
On Fri, Sep 07, 2018 at 02:50:34PM -0700, Ben Pfaff wrote: > On Wed, Sep 05, 2018 at 05:11:01PM -0700, Han Zhou wrote: > > On Wed, Sep 5, 2018 at 2:34 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Mon, Aug 13, 2018 at 10:47:59AM -0700, Han Zhou wrote: > > > > ovn-controller currently recomputes everything when there are any > > changes > > > > of input, which leads to high CPU usages and slow in end-to-end flow > > > > enforcement in response to changes. It even wastes CPU to recompute > > flows > > > > for unrelated inputs such as pinctrl events. > > > > > > Would you mind publishing this as a branch I can pull from? > > > > Hi Ben, > > > > The branch for the series is here: https://github.com/hzhou8/ovs/tree/ip12 > > Thanks. I am looking at it. > > I built the tip of the branch and saw only two warnings, from sparse: > > ../ovn/controller/ovn-controller.c:616:12: error: symbol 'sb_engine_node_names' was not declared. Should it be static? > ../ovn/controller/ovn-controller.c:641:12: error: symbol 'ovs_engine_node_names' was not declared. Should it be static? > > Usually this warning means that the function or variable should be > marked static. But in this case the variables can be deleted entirely: diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 99e901663ae7..6f22ba66eabd 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -613,12 +613,6 @@ enum sb_engine_node { #undef SB_NODE }; -const char *sb_engine_node_names[] = { -#define SB_NODE(NAME, NAME_STR) "SB_"NAME_STR, - SB_NODES -#undef SB_NODE -}; - #define SB_NODE_NAME(NAME) sb_engine_node_names[SB_##NAME] #define SB_NODE(NAME, NAME_STR) ENGINE_FUNC_SB(NAME); @@ -638,12 +632,6 @@ enum ovs_engine_node { #undef OVS_NODE }; -const char *ovs_engine_node_names[] = { -#define OVS_NODE(NAME, NAME_STR) "OVS_"NAME_STR, - OVS_NODES -#undef OVS_NODE -}; - #define OVS_NODE_NAME(NAME) ovs_engine_node_names[OVS_##NAME] #define OVS_NODE(NAME, NAME_STR) ENGINE_FUNC_OVS(NAME);
On Fri, Sep 7, 2018 at 2:51 PM Ben Pfaff <blp@ovn.org> wrote: > > On Fri, Sep 07, 2018 at 02:50:34PM -0700, Ben Pfaff wrote: > > On Wed, Sep 05, 2018 at 05:11:01PM -0700, Han Zhou wrote: > > > On Wed, Sep 5, 2018 at 2:34 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > On Mon, Aug 13, 2018 at 10:47:59AM -0700, Han Zhou wrote: > > > > > ovn-controller currently recomputes everything when there are any > > > changes > > > > > of input, which leads to high CPU usages and slow in end-to-end flow > > > > > enforcement in response to changes. It even wastes CPU to recompute > > > flows > > > > > for unrelated inputs such as pinctrl events. > > > > > > > > Would you mind publishing this as a branch I can pull from? > > > > > > Hi Ben, > > > > > > The branch for the series is here: https://github.com/hzhou8/ovs/tree/ip12 > > > > Thanks. I am looking at it. > > > > I built the tip of the branch and saw only two warnings, from sparse: > > > > ../ovn/controller/ovn-controller.c:616:12: error: symbol 'sb_engine_node_names' was not declared. Should it be static? > > ../ovn/controller/ovn-controller.c:641:12: error: symbol 'ovs_engine_node_names' was not declared. Should it be static? > > > > Usually this warning means that the function or variable should be > > marked static. > > But in this case the variables can be deleted entirely: > > diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c > index 99e901663ae7..6f22ba66eabd 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -613,12 +613,6 @@ enum sb_engine_node { > #undef SB_NODE > }; > > -const char *sb_engine_node_names[] = { > -#define SB_NODE(NAME, NAME_STR) "SB_"NAME_STR, > - SB_NODES > -#undef SB_NODE > -}; > - > #define SB_NODE_NAME(NAME) sb_engine_node_names[SB_##NAME] > > #define SB_NODE(NAME, NAME_STR) ENGINE_FUNC_SB(NAME); > @@ -638,12 +632,6 @@ enum ovs_engine_node { > #undef OVS_NODE > }; > > -const char *ovs_engine_node_names[] = { > -#define OVS_NODE(NAME, NAME_STR) "OVS_"NAME_STR, > - OVS_NODES > -#undef OVS_NODE > -}; > - > #define OVS_NODE_NAME(NAME) ovs_engine_node_names[OVS_##NAME] > > #define OVS_NODE(NAME, NAME_STR) ENGINE_FUNC_OVS(NAME); Hi Ben, Yes, I was using it in the beginning but refactored later and forgot to remove them. I will remove them in next version. I should try sparse for warnings. Thanks, Han
On Fri, Sep 7, 2018 at 4:59 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Fri, Sep 7, 2018 at 2:51 PM Ben Pfaff <blp@ovn.org> wrote: > > > > On Fri, Sep 07, 2018 at 02:50:34PM -0700, Ben Pfaff wrote: > > > On Wed, Sep 05, 2018 at 05:11:01PM -0700, Han Zhou wrote: > > > > On Wed, Sep 5, 2018 at 2:34 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > > On Mon, Aug 13, 2018 at 10:47:59AM -0700, Han Zhou wrote: > > > > > > ovn-controller currently recomputes everything when there are any > > > > changes > > > > > > of input, which leads to high CPU usages and slow in end-to-end flow > > > > > > enforcement in response to changes. It even wastes CPU to recompute > > > > flows > > > > > > for unrelated inputs such as pinctrl events. > > > > > > > > > > Would you mind publishing this as a branch I can pull from? > > > > > > > > Hi Ben, > > > > > > > > The branch for the series is here: https://github.com/hzhou8/ovs/tree/ip12 > > > > > > Thanks. I am looking at it. > > > > > > I built the tip of the branch and saw only two warnings, from sparse: > > > > > > ../ovn/controller/ovn-controller.c:616:12: error: symbol 'sb_engine_node_names' was not declared. Should it be static? > > > ../ovn/controller/ovn-controller.c:641:12: error: symbol 'ovs_engine_node_names' was not declared. Should it be static? > > > > > > Usually this warning means that the function or variable should be > > > marked static. > > > > But in this case the variables can be deleted entirely: > > > > diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c > > index 99e901663ae7..6f22ba66eabd 100644 > > --- a/ovn/controller/ovn-controller.c > > +++ b/ovn/controller/ovn-controller.c > > @@ -613,12 +613,6 @@ enum sb_engine_node { > > #undef SB_NODE > > }; > > > > -const char *sb_engine_node_names[] = { > > -#define SB_NODE(NAME, NAME_STR) "SB_"NAME_STR, > > - SB_NODES > > -#undef SB_NODE > > -}; > > - > > #define SB_NODE_NAME(NAME) sb_engine_node_names[SB_##NAME] > > > > #define SB_NODE(NAME, NAME_STR) ENGINE_FUNC_SB(NAME); > > @@ -638,12 +632,6 @@ enum ovs_engine_node { > > #undef OVS_NODE > > }; > > > > -const char *ovs_engine_node_names[] = { > > -#define OVS_NODE(NAME, NAME_STR) "OVS_"NAME_STR, > > - OVS_NODES > > -#undef OVS_NODE > > -}; > > - > > #define OVS_NODE_NAME(NAME) ovs_engine_node_names[OVS_##NAME] > > > > #define OVS_NODE(NAME, NAME_STR) ENGINE_FUNC_OVS(NAME); > > Hi Ben, > > Yes, I was using it in the beginning but refactored later and forgot to remove them. I will remove them in next version. > I should try sparse for warnings. > > Thanks, > Han Hi Ben, I updated the branch https://github.com/hzhou8/ovs/tree/ip12. Apart from removing the unused macros, there are 2 more fixes. The fixes are rebased into the ip12 branch. I kept the fixes on top of the original ip12 branch you reviewed in a new branch https://github.com/hzhou8/ovs/tree/ip12_fixes_for_v5, so that you can see the difference easily. Please take a look when you get time. I will rebase on master and submit v6 after getting your feedback for the whole series. Thanks, Han
ovn-controller currently recomputes everything when there are any changes of input, which leads to high CPU usages and slow in end-to-end flow enforcement in response to changes. It even wastes CPU to recompute flows for unrelated inputs such as pinctrl events. This patch series implements incremental processing in ovn-controller to solve above problems. There has been a similar attempt of solve the problem earlier but was reverted (see commit: 926c34fd). This patch series takes a different approach with an incremental processing engine, to make the dependencies clear and easier to maintain. For performance test results please find them in commit messages. The major change from v3 to v5 is better dependency maintenance. Firstly, it forces explicit dependency exposure for OVSDB tables used by any engine nodes, thanks for Ben's patches that supported passing individual tables. Since we are not passing IDL directly to the engine, missing dependency will either make compiling failed or result in assert failure in the first run. Secondly, it forces implicit dependency exposure for OVSDB table references through change tracking implementation, so that any referenced row change will cause the referencing node change, which will trigger proper change handling in the engine. Because of this capability, in change handler of port-binding, the logic is simplied a lot so that we don't have to re- consider multicast_group when port-binding is processed. Instead, change handler of multicast_group will automatically be triggered, and it will be incrementally processed, too. Some more notes: * In engine node any global variable access should be wrapped as engine node input. One example is mff_ovn_geneve, which is implemented as a separate engine node (an input node). However, currently there is no way to force this. So it depends on developers' awareness and code reviews. (Currently I don't see any global variables directly accessed from engine.) * Although dependencies between nodes are clear, the correctness of implementing change handler within an engine node relies on developers only - there is no mechanism yet to guarantee the outcome of change handler implementation is equivalent to full recompute, i.e. the run() function. * There can be further optimization when tables being referenced is not accessed, so that a change may not have to be handled by a specific engine node. This may be achieved by defining different versions of tables with different access permissions to referenced tables, as demonstrated by Ben's RFC: https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/348454.html. However, it seems not necessary at this point for most frequent use cases that we are targeting for performance improvement. For example, port-binding is referenced by multicast-group, but we do need to access port-binding from multicast-group in flow-output engine node, and both of them are incrementally processed. There is no other table references for the incremental processing we have in this patch series (port-binding, multicast-group, logical-flow, address-set, port-group). If we do have such use cases in the future, we may need to implement change tracking for different versions of the same table. --- v4 -> v5: * Fix the rebasing issue (compiling failure) in patch 6/20. v3 -> v4: * Rebased on master, and also reorganized patches by squashing some of them. * Forced explicit dependency exposure for OVSDB tables used by any engine node, thanks for Ben's patches that supported passing individual tables. * Forced implicit dependency exposure for OVSDB table references through change tracking. * Expose mff_ovn_geneve as a separate engine node. * Added Jakub's incremental test cases in the series, and added new tests for port-group incremental processing. * Added test result of address-set i-p from Mark Michelson in commit message. * Some general improvements in IDL (the first 3 patches) Han Zhou (18): ovsdb-idl.c: Remove a misleading comment for change tracking. ovsdb-idl.c: Update conditions when handling change tracking list. ovsdb-idlc.in: Support more interfaces for passing pointers of individual tables. ovsdb-idl.c: Track changes for table references. ovn-controller: Incremental processing engine ovn-controller: Track OVSDB changes ovn-controller: Initial use of incremental engine. ovn-controller: Incremental logical flow processing ovn-controller: runtime_data change handler for SB port-binding ovn-controller: port-binding incremental processing for physical flows ovn-controller: incremental processing for multicast group changes ovsdb-idl: Tracking - preserve data for deleted rows. ovn-controller: Split addr_sets from runtime_data. ovn-controller: Maintain resource references for logical flows. ovn-controller: Incremental processing for address-set changes. ovn-controller: Split port_groups from runtime_data. ovn-controller: Incremental processing for port-group changes. ovn-performance.at: Test port group incremental processing. Jakub Sitnicki (2): coverage: Add command for reading counter value. ovn: Test for full logical flow processing in ovn-controller. include/ovn/actions.h | 3 + include/ovn/expr.h | 5 +- lib/coverage-unixctl.man | 2 + lib/coverage.c | 42 ++ lib/ovsdb-idl-provider.h | 2 + lib/ovsdb-idl.c | 75 +- ovn/controller/bfd.c | 4 +- ovn/controller/binding.c | 108 ++- ovn/controller/binding.h | 7 + ovn/controller/encaps.c | 12 +- ovn/controller/lflow.c | 428 +++++++++++- ovn/controller/lflow.h | 100 ++- ovn/controller/ofctrl.c | 261 +++++-- ovn/controller/ofctrl.h | 33 +- ovn/controller/ovn-controller.c | 1454 +++++++++++++++++++++++++++++++++------ ovn/controller/physical.c | 194 ++++-- ovn/controller/physical.h | 19 +- ovn/lib/actions.c | 11 +- ovn/lib/automake.mk | 4 +- ovn/lib/expr.c | 21 +- ovn/lib/extend-table.c | 60 +- ovn/lib/extend-table.h | 16 +- ovn/lib/inc-proc-eng.c | 201 ++++++ ovn/lib/inc-proc-eng.h | 240 +++++++ ovn/utilities/ovn-trace.c | 2 +- ovsdb/ovsdb-idlc.in | 25 + tests/automake.mk | 3 +- tests/ovn-performance.at | 420 +++++++++++ tests/ovn.at | 74 ++ tests/test-ovn.c | 7 +- tests/testsuite.at | 1 + 31 files changed, 3382 insertions(+), 452 deletions(-) create mode 100644 ovn/lib/inc-proc-eng.c create mode 100644 ovn/lib/inc-proc-eng.h create mode 100644 tests/ovn-performance.at