[ovs-dev] [PATCH ovn v2] ovn-controller: Run I-P engine even when no SB txn is available.
Han Zhou
hzhou at ovn.org
Thu Dec 19 18:45:06 UTC 2019
On Mon, Dec 9, 2019 at 12:12 AM Dumitru Ceara <dceara at 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 at ovn.org>
> CC: Numan Siddique <numans at ovn.org>
> Signed-off-by: Dumitru Ceara <dceara at 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.
More information about the dev
mailing list