| Message ID | 1575879138-22156-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,v2] ovn-controller: Run I-P engine even when no SB txn is available. | expand |
On Mon, Dec 9, 2019 at 12:12 AM Dumitru Ceara <dceara@redhat.com> wrote: > > If the last ovn-controller main loop run started a transaction to the > Southbound DB and the transaction is still in progress, ovn-controller > will not call engine_run(). In case there were changes to the DB, > engine_need_run() would return true which would trigger an immediate > forced recompute in the next processing loop iteration. > > However, there are scenarios when updates can be processed incrementally > even if no Southbound DB transaction is available. One example, often > seen in the field, is when the MAC_Binding table is updated. Currently > such an update received while a transaction is still committing would > trigger a forced recompute. > > This patch performs only incremental processing when the SB DB txn > is NULL, which means executing change_handler() methods > only, without calling the run() methods of the I-P engine nodes. > Assuming that some change handlers need to update the DB and that > some don't, if the SB DB txn is NULL: > - if the incoming change doesn't trigger a SB DB update (currently true > for all existing change handlers) then the change handler might > complete without aborting if it doesn't trigger a run() of the node. > - if the incoming change tries to perform a SB DB update, the change > handler should return false (SB DB txn is NULL) triggering the engine > to call run() and abort computation. > > CC: Han Zhou <hzhou@ovn.org> > CC: Numan Siddique <numans@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > v2: > - Address Han's comments: > - Remove unnecessary poll_immediate_wake() that could trigger a busy > loop until the transaction is committed. > - Document the change_handler requirements regarding using the txn > pointers returned by engine_get_context(). > - Rephrase commit message. > - Rebase. > --- > controller/ovn-controller.c | 8 ++++++++ > lib/inc-proc-eng.h | 15 ++++++++++++++- > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 5874776..e47687b 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2100,6 +2100,14 @@ main(int argc, char *argv[]) > } else { > engine_run(true); > } > + } else { > + /* Even if there's no SB DB transaction available, > + * try to run the engine so that we can handle any > + * incremental changes that don't require a recompute. > + * If a recompute is required, the engine will abort, > + * triggerring a full run in the next iteration. > + */ > + engine_run(false); > } > stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, > time_msec()); > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 5b92971..780c3cd 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -84,6 +84,10 @@ struct engine_node_input { > * evaluated against all the other inputs. Returns: > * - true: if change can be handled > * - false: if change cannot be handled (indicating full recompute needed) > + * A change handler can also call engine_get_context() but it must make > + * sure the txn pointers returned by it are non-NULL. In case the change > + * handler needs to use the txn pointers returned by engine_get_context(), > + * and the pointers are NULL, the change handler MUST return false. > */ > bool (*change_handler)(struct engine_node *node, void *data); > }; > @@ -133,6 +137,9 @@ struct engine_node { > > /* Fully processes all inputs of this node and regenerates the data > * of this node. The pointer to the node's data is passed as argument. > + * 'run' handlers can also call engine_get_context() and the > + * implementation guarantees that the txn pointers returned > + * engine_get_context() are not NULL and valid. > */ > void (*run)(struct engine_node *node, void *data); > > @@ -189,7 +196,13 @@ void engine_add_input(struct engine_node *node, struct engine_node *input, > * iteration, and the change can't be tracked across iterations */ > void engine_set_force_recompute(bool val); > > -const struct engine_context * engine_get_context(void); > +/* Return the current engine_context. The values in the context can be NULL > + * if the engine is run with allow_recompute == false in the current > + * iteration. > + * Therefore, it is the responsibility of the caller to check the context > + * values when called from change handlers. > + */ > +const struct engine_context *engine_get_context(void); > > void engine_set_context(const struct engine_context *); > > -- > 1.8.3.1 > Thanks! I applied to master.
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 5874776..e47687b 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2100,6 +2100,14 @@ main(int argc, char *argv[]) } else { engine_run(true); } + } else { + /* Even if there's no SB DB transaction available, + * try to run the engine so that we can handle any + * incremental changes that don't require a recompute. + * If a recompute is required, the engine will abort, + * triggerring a full run in the next iteration. + */ + engine_run(false); } stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, time_msec()); diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 5b92971..780c3cd 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -84,6 +84,10 @@ struct engine_node_input { * evaluated against all the other inputs. Returns: * - true: if change can be handled * - false: if change cannot be handled (indicating full recompute needed) + * A change handler can also call engine_get_context() but it must make + * sure the txn pointers returned by it are non-NULL. In case the change + * handler needs to use the txn pointers returned by engine_get_context(), + * and the pointers are NULL, the change handler MUST return false. */ bool (*change_handler)(struct engine_node *node, void *data); }; @@ -133,6 +137,9 @@ struct engine_node { /* Fully processes all inputs of this node and regenerates the data * of this node. The pointer to the node's data is passed as argument. + * 'run' handlers can also call engine_get_context() and the + * implementation guarantees that the txn pointers returned + * engine_get_context() are not NULL and valid. */ void (*run)(struct engine_node *node, void *data); @@ -189,7 +196,13 @@ void engine_add_input(struct engine_node *node, struct engine_node *input, * iteration, and the change can't be tracked across iterations */ void engine_set_force_recompute(bool val); -const struct engine_context * engine_get_context(void); +/* Return the current engine_context. The values in the context can be NULL + * if the engine is run with allow_recompute == false in the current + * iteration. + * Therefore, it is the responsibility of the caller to check the context + * values when called from change handlers. + */ +const struct engine_context *engine_get_context(void); void engine_set_context(const struct engine_context *);
If the last ovn-controller main loop run started a transaction to the Southbound DB and the transaction is still in progress, ovn-controller will not call engine_run(). In case there were changes to the DB, engine_need_run() would return true which would trigger an immediate forced recompute in the next processing loop iteration. However, there are scenarios when updates can be processed incrementally even if no Southbound DB transaction is available. One example, often seen in the field, is when the MAC_Binding table is updated. Currently such an update received while a transaction is still committing would trigger a forced recompute. This patch performs only incremental processing when the SB DB txn is NULL, which means executing change_handler() methods only, without calling the run() methods of the I-P engine nodes. Assuming that some change handlers need to update the DB and that some don't, if the SB DB txn is NULL: - if the incoming change doesn't trigger a SB DB update (currently true for all existing change handlers) then the change handler might complete without aborting if it doesn't trigger a run() of the node. - if the incoming change tries to perform a SB DB update, the change handler should return false (SB DB txn is NULL) triggering the engine to call run() and abort computation. CC: Han Zhou <hzhou@ovn.org> CC: Numan Siddique <numans@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v2: - Address Han's comments: - Remove unnecessary poll_immediate_wake() that could trigger a busy loop until the transaction is committed. - Document the change_handler requirements regarding using the txn pointers returned by engine_get_context(). - Rephrase commit message. - Rebase. --- controller/ovn-controller.c | 8 ++++++++ lib/inc-proc-eng.h | 15 ++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-)