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,
> 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.
> 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) {
> + 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.
> {
> - 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);
}
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);
Should we also add a new command to list the engines? E.g.:
$ ovn-appctl inc-engine/list-engines
flow-engine
meter-engine
etc.
> + 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)
> {
> - 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);
> - free(engine_nodes);
> - engine_nodes = NULL;
> - engine_n_nodes = 0;
> + e->engine_n_nodes = 0;
This is not needed.
> + 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.
> + /* 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.
> + 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
