On Dec 20, Dumitru Ceara wrote:
> On 12/9/21 17:47, Lorenzo Bianconi wrote:
> > Remove global state variable and move move inc-proc code in an isolated
> > strucuture. This is a preliminary patch to add the capability to run
> > multiple inc-proc engines.
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
>
> Hi Lorenzo,
Hi Dumitru,
thx for the review.
>
> > Changes since v1:
> > - fix unixctl commands for IP engine.
> > ---
> > controller/ovn-controller.c | 65 ++++++------
> > lib/inc-proc-eng.c | 199 ++++++++++++++++++++++--------------
> > lib/inc-proc-eng.h | 40 ++++++--
> > northd/en-lflow.c | 2 +-
> > northd/en-northd.c | 2 +-
> > northd/inc-proc-northd.c | 28 ++---
> > 6 files changed, 202 insertions(+), 134 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 5069aedfc..9f5e55bac 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
>
> [...]
>
> > @@ -4152,9 +4157,11 @@ inject_pkt(struct unixctl_conn *conn, int argc
> > OVS_UNUSED,
> >
> > static void
> > engine_recompute_cmd(struct unixctl_conn *conn OVS_UNUSED, 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);
>
> Nit: This can be engine_trigger_recompute(arg) directly.
ack, I will fix it in v3.
>
> > unixctl_command_reply(conn, NULL);
> > }
> >
> > @@ -4166,7 +4173,7 @@ lflow_cache_flush_cmd(struct unixctl_conn *conn
> > OVS_UNUSED,
> > VLOG_INFO("User triggered lflow cache flush.");
> > struct lflow_output_persistent_data *fo_pd = arg_;
> > lflow_cache_flush(fo_pd->lflow_cache);
> > - engine_set_force_recompute(true);
> > + engine_set_force_recompute(flow_engine, true);
> > poll_immediate_wake();
> > unixctl_command_reply(conn, NULL);
> > }
> > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> > index 2958a55e3..e7b3e9d2d 100644
> > --- a/lib/inc-proc-eng.c
> > +++ b/lib/inc-proc-eng.c
> > @@ -33,12 +33,7 @@
> >
> > VLOG_DEFINE_THIS_MODULE(inc_proc_eng);
> >
> > -static bool engine_force_recompute = false;
> > -static bool engine_run_aborted = false;
> > -static const struct engine_context *engine_context;
> > -
> > -static struct engine_node **engine_nodes;
> > -static size_t engine_n_nodes;
> > +static struct ovs_list engines = OVS_LIST_INITIALIZER(&engines);
> >
> > static const char *engine_node_state_name[EN_STATE_MAX] = {
> > [EN_STALE] = "Stale",
> > @@ -52,21 +47,21 @@ engine_recompute(struct engine_node *node, bool allowed,
> > const char *reason_fmt, ...) OVS_PRINTF_FORMAT(3, 4);
> >
> > void
> > -engine_set_force_recompute(bool val)
> > +engine_set_force_recompute(struct engine *e, bool val)
> > {
> > - engine_force_recompute = val;
> > + e->engine_force_recompute = val;
> > }
> >
> > const struct engine_context *
> > -engine_get_context(void)
> > +engine_get_context(struct engine *e)
> > {
> > - return engine_context;
> > + return e->engine_context;
> > }
> >
> > void
> > -engine_set_context(const struct engine_context *ctx)
> > +engine_set_context(struct engine *e, const struct engine_context *ctx)
> > {
> > - engine_context = ctx;
> > + e->engine_context = ctx;
> > }
> >
> > /* Builds the topologically sorted 'sorted_nodes' array starting from
> > @@ -113,30 +108,53 @@ static void
> > engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED)
> > {
> > - for (size_t i = 0; i < engine_n_nodes; i++) {
> > - struct engine_node *node = engine_nodes[i];
> > + const char *target = argc == 2 ? argv[1] : NULL;
> > + struct ds reply = DS_EMPTY_INITIALIZER;
> > + struct engine *e;
> > +
> > + ds_put_format(&reply, "no %s engine found", target ? target : "");
> > + LIST_FOR_EACH (e, node, &engines) {
> > + for (size_t i = 0; i < e->engine_n_nodes; i++) {
> > + struct engine_node *node = e->engine_nodes[i];
> >
> > - memset(&node->stats, 0, sizeof node->stats);
> > + if (target && strcmp(target, e->name)) {
> > + continue;
> > + }
> > + memset(&node->stats, 0, sizeof node->stats);
> > + ds_clear(&reply);
> > + }
> > }
> > - unixctl_command_reply(conn, NULL);
> > +
> > + unixctl_command_reply(conn, ds_cstr(&reply));
> > + ds_destroy(&reply);
> > }
> >
> > static void
> > -engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > +engine_dump_stats(struct unixctl_conn *conn, int argc,
> > const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED)
> > {
> > + const char *target = argc == 2 ? argv[1] : NULL;
> > struct ds dump = DS_EMPTY_INITIALIZER;
> > + struct engine *e;
> >
> > - for (size_t i = 0; i < engine_n_nodes; i++) {
> > - struct engine_node *node = engine_nodes[i];
> > + LIST_FOR_EACH (e, node, &engines) {
> > + for (size_t i = 0; i < e->engine_n_nodes; i++) {
> > + struct engine_node *node = e->engine_nodes[i];
> >
> > - ds_put_format(&dump,
> > - "Node: %s\n"
> > - "- recompute: %12"PRIu64"\n"
> > - "- compute: %12"PRIu64"\n"
> > - "- abort: %12"PRIu64"\n",
> > - node->name, node->stats.recompute,
> > - node->stats.compute, node->stats.abort);
> > + if (target && strcmp(target, e->name)) {
> > + continue;
> > + }
> > + ds_put_format(&dump,
> > + "Node: %s\n"
> > + "- recompute: %12"PRIu64"\n"
> > + "- compute: %12"PRIu64"\n"
> > + "- abort: %12"PRIu64"\n",
> > + node->name, node->stats.recompute,
> > + node->stats.compute, node->stats.abort);
> > + }
> > + }
> > + if (!dump.length) {
>
> It's probably better to not use dynamic string internals. I'd rewrite
> this as:
>
> if (ds_last(&dump) == EOF) {
ack, I will fix it in v3.
>
> > + ds_put_format(&dump, "no %s engine found", target ? target : "");
> > }
> > unixctl_command_reply(conn, ds_cstr(&dump));
> >
> > @@ -148,48 +166,69 @@ engine_trigger_recompute_cmd(struct unixctl_conn
> > *conn, int argc OVS_UNUSED,
> > const char *argv[] OVS_UNUSED,
> > void *arg OVS_UNUSED)
> > {
> > - engine_trigger_recompute();
> > - unixctl_command_reply(conn, NULL);
> > + const char *target = argc == 2 ? argv[1] : NULL;
> > + struct ds reply = DS_EMPTY_INITIALIZER;
> > + struct engine *e;
> > +
> > + ds_put_format(&reply, "no %s engine found", target ? target : "");
> > + LIST_FOR_EACH (e, node, &engines) {
> > + if (target && strcmp(target, e->name)) {
> > + continue;
> > + }
> > + engine_trigger_recompute(e);
> > + ds_clear(&reply);
> > + }
> > +
> > + unixctl_command_reply(conn, ds_cstr(&reply));
> > + ds_destroy(&reply);
> > }
> >
> > -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, const char *name)
>
> It's probably more clear if we return "struct engine *" from this
> function instead of passing "struct **pe". We could also rename it to
> engine_new(..), I think.
ack, I will fix it in v3.
>
> > {
> > - 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);
> > + e->name = name;
> >
> > - 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,
> > + unixctl_command_register("inc-engine/show-stats", "", 0, 1,
> > engine_dump_stats, NULL);
> > - unixctl_command_register("inc-engine/clear-stats", "", 0, 0,
> > + unixctl_command_register("inc-engine/clear-stats", "", 0, 1,
> > engine_clear_stats, NULL);
> > - unixctl_command_register("inc-engine/recompute", "", 0, 0,
> > + unixctl_command_register("inc-engine/recompute", "", 0, 1,
> > engine_trigger_recompute_cmd, NULL);
>
> It's a bit confusing to call unixctl_command_register() here now.
> It will not do anything except the first time an engine is added. We
> should probably use a constructor and move this part to a separate function:
>
> OVS_CONSTRUCTOR(inc_proc_eng_init)
> {
> unixctl_command_register("inc-engine/show-stats", "", 0, 1,
> engine_dump_stats, NULL);
> unixctl_command_register("inc-engine/clear-stats", "", 0, 1,
> engine_clear_stats, NULL);
> unixctl_command_register("inc-engine/recompute", "", 0, 1,
> engine_trigger_recompute_cmd, NULL);
> }
ack, I will fix it in v3.
>
> We also need to update the usage strings for these commands, e.g.:
>
> unixctl_command_register("inc-engine/show-stats",
> "[engine]", 0, 1,
> engine_dump_stats, NULL);
ack, I will fix it in v3.
>
> Should we also add a new command to list the engines? E.g.:
>
> $ ovn-appctl inc-engine/list-engines
> flow-engine
> meter-engine
> etc.
ack, I will add it in v3.
>
> > + ovs_list_push_back(&engines, &e->node);
> > + *pe = e;
> > }
> >
> > void
> > -engine_cleanup(void)
> > +engine_cleanup(struct engine **pe)
>
> If we change engine_init() to return the new engine then we should also
> change this to:
>
> void
> engine_cleanup(struct engine *pe)
ack, I will fix it in v3.
>
> > {
> > - for (size_t i = 0; i < engine_n_nodes; i++) {
> > - if (engine_nodes[i]->clear_tracked_data) {
> > - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data);
> > + struct engine *e = *pe;
> > + for (size_t i = 0; i < e->engine_n_nodes; i++) {
> > + if (e->engine_nodes[i]->clear_tracked_data) {
> > + e->engine_nodes[i]->clear_tracked_data(
> > + e->engine_nodes[i]->data);
> > }
> >
> > - if (engine_nodes[i]->cleanup) {
> > - engine_nodes[i]->cleanup(engine_nodes[i]->data);
> > + if (e->engine_nodes[i]->cleanup) {
> > + e->engine_nodes[i]->cleanup(e->engine_nodes[i]->data);
> > }
> > - free(engine_nodes[i]->data);
> > + free(e->engine_nodes[i]->data);
> > }
>
> We need to remove the engine from the list of engines:
>
> ovs_list_remove(&e->node);
ditto.
>
> > - free(engine_nodes);
> > - engine_nodes = NULL;
> > - engine_n_nodes = 0;
> > + e->engine_n_nodes = 0;
>
> This is not needed.
ditto.
>
> > + free(e->engine_nodes);
> > + *pe = NULL;
> > }
> >
>
> [...]
>
> > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> > index 9bfab1f7c..7f59dfa63 100644
> > --- a/lib/inc-proc-eng.h
> > +++ b/lib/inc-proc-eng.h
> > @@ -67,6 +67,7 @@
> > #include <stdint.h>
> >
> > #include "compiler.h"
> > +#include "openvswitch/list.h"
> >
> > struct engine_context {
> > struct ovsdb_idl_txn *ovs_idl_txn;
> > @@ -122,6 +123,8 @@ struct engine_stats {
> > };
> >
> > struct engine_node {
> > + struct engine *e;
> > +
> > /* A unique name for each node. */
> > char *name;
> >
> > @@ -173,30 +176,45 @@ struct engine_node {
> > struct engine_stats stats;
> > };
> >
> > +struct engine {
> > + struct ovs_list node;
>
> Nit: Please add an empty line.
ditto.
>
> > + /* A unique name for each node. */
>
> Nit: or remove this comment, seems a bit redundant and it's actually the
> engine's name, not the node's.
ditto.
>
> > + const char *name;
> > +
> > + struct engine_node **engine_nodes;
> > + size_t engine_n_nodes;
> > +
> > + bool engine_force_recompute;
> > + bool engine_run_aborted;
> > +
> > + const struct engine_context *engine_context;
> > +};
>
> Thanks,
> Dumitru
Regards,
Lorenzo
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
