[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