Hi Lorenzo,

Typo in the subject: "strucuture".

On 12/3/21 19:53, Lorenzo Bianconi wrote:
> Remove global state variables and move move inc-proc code in an isolated
> structure. This is a preliminary patch to add the capability to run
> multiple inc-proc engines.

Overall this looks OK to me; there is however an issue with the unixctl
commands, please see below.

> 
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
>  controller/ovn-controller.c |  65 +++++++++-------
>  lib/inc-proc-eng.c          | 151 +++++++++++++++++++-----------------
>  lib/inc-proc-eng.h          |  35 ++++++---
>  northd/en-lflow.c           |   2 +-
>  northd/en-northd.c          |   2 +-
>  northd/inc-proc-northd.c    |  28 +++----
>  6 files changed, 155 insertions(+), 128 deletions(-)
> 

[...]

> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> index 2958a55e3..86d2df520 100644
> --- a/lib/inc-proc-eng.c
> +++ b/lib/inc-proc-eng.c

[...]

> @@ -145,51 +141,58 @@ engine_dump_stats(struct unixctl_conn *conn, int argc 
> OVS_UNUSED,
>  
>  static void
>  engine_trigger_recompute_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                             const char *argv[] OVS_UNUSED,
> -                             void *arg OVS_UNUSED)
> +                             const char *argv[] OVS_UNUSED, void *arg)
>  {
> -    engine_trigger_recompute();
> +    struct engine *e = arg;
> +
> +    engine_trigger_recompute(e);
>      unixctl_command_reply(conn, NULL);
>  }
>  
> -void
> -engine_init(struct engine_node *node, struct engine_arg *arg)
> +void engine_init(struct engine **pe, struct engine_node *node,
> +                 struct engine_arg *arg)
>  {
> -    engine_nodes = engine_get_nodes(node, &engine_n_nodes);
> +    struct engine *e = xzalloc(sizeof *e);
> +
> +    e->engine_nodes = engine_get_nodes(node, &e->engine_n_nodes);
>  
> -    for (size_t i = 0; i < engine_n_nodes; i++) {
> -        if (engine_nodes[i]->init) {
> -            engine_nodes[i]->data =
> -                engine_nodes[i]->init(engine_nodes[i], arg);
> +    for (size_t i = 0; i < e->engine_n_nodes; i++) {
> +        if (e->engine_nodes[i]->init) {
> +            e->engine_nodes[i]->data =
> +                e->engine_nodes[i]->init(e->engine_nodes[i], arg);
>          } else {
> -            engine_nodes[i]->data = NULL;
> +            e->engine_nodes[i]->data = NULL;
>          }
> +        e->engine_nodes[i]->e = e;
>      }
>  
>      unixctl_command_register("inc-engine/show-stats", "", 0, 0,
> -                             engine_dump_stats, NULL);
> +                             engine_dump_stats, e);
>      unixctl_command_register("inc-engine/clear-stats", "", 0, 0,
> -                             engine_clear_stats, NULL);
> +                             engine_clear_stats, e);
>      unixctl_command_register("inc-engine/recompute", "", 0, 0,
> -                             engine_trigger_recompute_cmd, NULL);
> +                             engine_trigger_recompute_cmd, e);

This won't work as expected if there are multiple incremental processing
engines.  The commands will only be registered for the first one.  We
need to find a different way to do this.

Regards,
Dumitru

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to