Re: [ovs-dev] [PATCH v2 ovn] inc-proc-eng: move inc-proc code in an isolated strucuture
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
> > ---
>
> 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();
> >
> > 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(, "no %s engine found", target ? target : "");
> > +LIST_FOR_EACH (e, node, ) {
> > +for (size_t i = 0; i < e->engine_n_nodes; i++) {
> > +struct engine_node *node = e->engine_nodes[i];
> >
> > -memset(>stats, 0, sizeof node->stats);
> > +if (target && strcmp(target, e->name)) {
> > +continue;
> > +}
> > +memset(>stats, 0, sizeof node->stats);
> > +ds_clear();
> > +}
> > }
> > -unixctl_command_reply(conn, NULL);
> > +
> > +unixctl_command_reply(conn, ds_cstr());
> > +ds_destroy();
> > }
> >
> > 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 =
Re: [ovs-dev] [PATCH v2 ovn] inc-proc-eng: move inc-proc code in an isolated strucuture
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
> ---
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();
>
> 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(, "no %s engine found", target ? target : "");
> +LIST_FOR_EACH (e, node, ) {
> +for (size_t i = 0; i < e->engine_n_nodes; i++) {
> +struct engine_node *node = e->engine_nodes[i];
>
> -memset(>stats, 0, sizeof node->stats);
> +if (target && strcmp(target, e->name)) {
> +continue;
> +}
> +memset(>stats, 0, sizeof node->stats);
> +ds_clear();
> +}
> }
> -unixctl_command_reply(conn, NULL);
> +
> +unixctl_command_reply(conn, ds_cstr());
> +ds_destroy();
> }
>
> 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, ) {
> +for (size_t i = 0; i < e->engine_n_nodes; i++) {
> +struct engine_node *node = e->engine_nodes[i];
>
> -ds_put_format(,
> - "Node: %s\n"
> -
[ovs-dev] [PATCH v2 ovn] inc-proc-eng: move inc-proc code in an isolated strucuture
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
---
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
@@ -114,6 +114,8 @@ static unixctl_cb_func debug_delay_nb_cfg_report;
#define OVS_NB_CFG_TS_NAME "ovn-nb-cfg-ts"
#define OVS_STARTUP_TS_NAME "ovn-startup-ts"
+static struct engine *flow_engine;
+
static char *parse_options(int argc, char *argv[]);
OVS_NO_RETURN static void usage(void);
@@ -557,7 +559,7 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl
*ovnsb_idl,
}
if (reset_ovnsb_idl_min_index && *reset_ovnsb_idl_min_index) {
VLOG_INFO("Resetting southbound database cluster state");
-engine_set_force_recompute(true);
+engine_set_force_recompute(flow_engine, true);
ovsdb_idl_reset_min_index(ovnsb_idl);
*reset_ovnsb_idl_min_index = false;
}
@@ -1011,7 +1013,8 @@ en_ofctrl_is_connected_cleanup(void *data OVS_UNUSED)
static void
en_ofctrl_is_connected_run(struct engine_node *node, void *data)
{
-struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx;
+struct controller_engine_ctx *ctrl_ctx =
+engine_get_context(flow_engine)->client_ctx;
struct ed_type_ofctrl_is_connected *of_data = data;
if (of_data->connected != ofctrl_is_connected()) {
of_data->connected = !of_data->connected;
@@ -1226,10 +1229,11 @@ init_binding_ctx(struct engine_node *node,
engine_get_input("SB_port_binding", node),
"datapath");
-struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx;
+struct controller_engine_ctx *ctrl_ctx =
+engine_get_context(flow_engine)->client_ctx;
-b_ctx_in->ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn;
-b_ctx_in->ovs_idl_txn = engine_get_context()->ovs_idl_txn;
+b_ctx_in->ovnsb_idl_txn = engine_get_context(flow_engine)->ovnsb_idl_txn;
+b_ctx_in->ovs_idl_txn = engine_get_context(flow_engine)->ovs_idl_txn;
b_ctx_in->sbrec_datapath_binding_by_key = sbrec_datapath_binding_by_key;
b_ctx_in->sbrec_port_binding_by_datapath = sbrec_port_binding_by_datapath;
b_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
@@ -2387,7 +2391,8 @@ en_lflow_output_run(struct engine_node *node, void *data)
lflow_conj_ids_clear(>conj_ids);
}
-struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx;
+struct controller_engine_ctx *ctrl_ctx =
+engine_get_context(flow_engine)->client_ctx;
fo->pd.lflow_cache = ctrl_ctx->lflow_cache;
@@ -3040,7 +3045,7 @@ check_northd_version(struct ovsdb_idl *ovs_idl, struct
ovsdb_idl *ovnsb_idl,
* full recompute.
*/
if (version_mismatch) {
-engine_set_force_recompute(true);
+engine_set_force_recompute(flow_engine, true);
}
version_mismatch = false;
return true;
@@ -3344,7 +3349,7 @@ main(int argc, char *argv[])
.sb_idl = ovnsb_idl_loop.idl,
.ovs_idl = ovs_idl_loop.idl,
};
-engine_init(_flow_output, _arg);
+engine_init(_engine, _flow_output, _arg, "flow_engine");
engine_ovsdb_node_add_index(_sb_chassis, "name", sbrec_chassis_by_name);
engine_ovsdb_node_add_index(_sb_multicast_group, "name_datapath",
@@ -3396,7 +3401,7 @@ main(int argc, char *argv[])
unixctl_command_register("recompute", "[deprecated]", 0, 0,
engine_recompute_cmd,
- NULL);
+ flow_engine);
unixctl_command_register("lflow-cache/flush", "", 0, 0,
lflow_cache_flush_cmd,
_output_data->pd);
@@ -3480,7 +3485,7 @@ main(int argc, char *argv[])
goto loop_done;
}
-engine_init_run();
+engine_init_run(flow_engine);
struct ovsdb_idl_txn *ovs_idl_txn = ovsdb_idl_loop_run(_idl_loop);
unsigned int new_ovs_cond_seqno
@@ -3488,7 +3493,7 @@ main(int argc, char *argv[])
if (new_ovs_cond_seqno != ovs_cond_seqno) {
if (!new_ovs_cond_seqno) {
VLOG_INFO("OVS IDL reconnected, force recompute.");
-engine_set_force_recompute(true);
+engine_set_force_recompute(flow_engine, true);
}
ovs_cond_seqno =
