| Message ID | 20240924133300.329879-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] inc-engine: Adjust the force recompute API. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Tue, Sep 24, 2024 at 9:33 AM Ales Musil <amusil@redhat.com> wrote: > > There are cases when we need to wake the thread immediately after > setting force recompute. This is mainly in error handling that > happens after engine_run() call. In order to achieve that make > the API more ergonomic with parameters to force the wake if needed. > > Reported-at: https://issues.redhat.com/browse/FDP-753 > Signed-off-by: Ales Musil <amusil@redhat.com> The function engine_set_force_recompute(bool) gives the notion that engine_set_force_recompute(true) -> set the force recompute to true and engine_set_force_recompute(false) -> set the force recompute to false which is the case now (without your patch). Changing the meaning of the input param bool to "immediate" doesn't seem obvious to me. Does it make sense to have the below functions instead ? void engine_set_force_recompute(void); void engine_set_force_recompute_immediate(void); void engine_clear_force_recompute(void); Thanks Numan > --- > controller/ovn-controller.c | 17 +++++++---------- > lib/inc-proc-eng.c | 20 +++++++++++++++++--- > lib/inc-proc-eng.h | 15 ++++++++++++--- > northd/inc-proc-northd.c | 14 ++------------ > northd/inc-proc-northd.h | 13 ++++++++++++- > northd/ovn-northd.c | 15 +++++++-------- > 6 files changed, 57 insertions(+), 37 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 168167b1a..f21d46fc3 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -641,7 +641,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(false); > ovsdb_idl_reset_min_index(ovnsb_idl); > *reset_ovnsb_idl_min_index = false; > } > @@ -4768,7 +4768,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(false); > } > version_mismatch = false; > return true; > @@ -5382,7 +5382,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(false); > } > ovs_cond_seqno = new_ovs_cond_seqno; > } > @@ -5399,7 +5399,7 @@ main(int argc, char *argv[]) > if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) { > if (!new_ovnsb_cond_seqno) { > VLOG_INFO("OVNSB IDL reconnected, force recompute."); > - engine_set_force_recompute(true); > + engine_set_force_recompute(false); > } > ovnsb_cond_seqno = new_ovnsb_cond_seqno; > } > @@ -5488,7 +5488,7 @@ main(int argc, char *argv[]) > br_int_remote.target, > br_int_remote.probe_interval)) { > VLOG_INFO("OVS feature set changed, force recompute."); > - engine_set_force_recompute(true); > + engine_set_force_recompute(false); > > struct ed_type_lflow_output *lflow_out_data = > engine_get_internal_data(&en_lflow_output); > @@ -5510,7 +5510,7 @@ main(int argc, char *argv[]) > > VLOG_INFO_RL(&rl, "OVS OpenFlow connection reconnected," > "force recompute."); > - engine_set_force_recompute(true); > + engine_set_force_recompute(false); > } > > if (chassis && ovs_feature_set_discovered()) { > @@ -5738,7 +5738,6 @@ main(int argc, char *argv[]) > VLOG_DBG("engine did not run, force recompute next time: " > "br_int %p, chassis %p", br_int, chassis); > engine_set_force_recompute(true); > - poll_immediate_wake(); > } else { > VLOG_DBG("engine did not run, and it was not needed" > " either: br_int %p, chassis %p", > @@ -5748,9 +5747,8 @@ main(int argc, char *argv[]) > VLOG_DBG("engine was canceled, force recompute next time: " > "br_int %p, chassis %p", br_int, chassis); > engine_set_force_recompute(true); > - poll_immediate_wake(); > } else { > - engine_set_force_recompute(false); > + engine_clear_force_recompute(); > } > > store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private, > @@ -6138,7 +6136,6 @@ lflow_cache_flush_cmd(struct unixctl_conn *conn OVS_UNUSED, > struct lflow_output_persistent_data *fo_pd = arg_; > lflow_cache_flush(fo_pd->lflow_cache); > engine_set_force_recompute(true); > - poll_immediate_wake(); > unixctl_command_reply(conn, NULL); > } > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index a01440bb4..5748727b5 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -54,9 +54,24 @@ 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(bool immediate) > { > - engine_force_recompute = val; > + engine_force_recompute = true; > + if (immediate) { > + poll_immediate_wake(); > + } > +} > + > +void > +engine_clear_force_recompute(void) > +{ > + engine_force_recompute = false; > +} > + > +bool > +engine_get_force_recompute(void) > +{ > + return engine_force_recompute; > } > > const struct engine_context * > @@ -556,5 +571,4 @@ engine_trigger_recompute(void) > { > VLOG_INFO("User triggered force recompute."); > engine_set_force_recompute(true); > - poll_immediate_wake(); > } > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 5bb3b8f3e..db80ae975 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -297,11 +297,20 @@ void *engine_get_input_data(const char *input_name, struct engine_node *); > void engine_add_input(struct engine_node *node, struct engine_node *input, > bool (*change_handler)(struct engine_node *, void *)); > > -/* Force the engine to recompute everything if set to true. It is used > +/* Force the engine to recompute everything. It is used > * in circumstances when we are not sure there is change or not, or > * when there is change but the engine couldn't be executed in that > - * iteration, and the change can't be tracked across iterations */ > -void engine_set_force_recompute(bool val); > + * iteration, and the change can't be tracked across iterations. > + * The immediate makes sure that the loop is woken up immediately > + * so the next engine run is not delayed. */ > +void engine_set_force_recompute(bool immediate); > + > +/* Clear the force flag for the next run so the engine does the > + * usual processing without forced full recompute. */ > +void engine_clear_force_recompute(void); > + > +/* Returns whether next engine_run() is forced to rempute. */ > +bool engine_get_force_recompute(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 > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 1f79916a5..667a13422 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -428,14 +428,6 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, > int64_t start = time_msec(); > engine_init_run(); > > - /* Force a full recompute if instructed to, for example, after a NB/SB > - * reconnect event. However, make sure we don't overwrite an existing > - * force-recompute request if 'recompute' is false. > - */ > - if (ctx->recompute) { > - engine_set_force_recompute(ctx->recompute); > - } > - > struct engine_context eng_ctx = { > .ovnnb_idl_txn = ovnnb_txn, > .ovnsb_idl_txn = ovnsb_txn, > @@ -448,16 +440,14 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, > if (engine_need_run()) { > VLOG_DBG("engine did not run, force recompute next time."); > engine_set_force_recompute(true); > - poll_immediate_wake(); > } else { > VLOG_DBG("engine did not run, and it was not needed"); > } > } else if (engine_canceled()) { > VLOG_DBG("engine was canceled, force recompute next time."); > engine_set_force_recompute(true); > - poll_immediate_wake(); > } else { > - engine_set_force_recompute(false); > + engine_clear_force_recompute(); > } > > int64_t now = time_msec(); > @@ -477,7 +467,7 @@ void inc_proc_northd_cleanup(void) > bool > inc_proc_northd_can_run(struct northd_engine_context *ctx) > { > - if (ctx->recompute || time_msec() >= ctx->next_run_ms || > + if (engine_get_force_recompute() || time_msec() >= ctx->next_run_ms || > ctx->nb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS || > ctx->sb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS) { > return true; > diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h > index a2b9b7fdb..7c2cb2e7a 100644 > --- a/northd/inc-proc-northd.h > +++ b/northd/inc-proc-northd.h > @@ -13,7 +13,6 @@ struct northd_engine_context { > uint64_t nb_idl_duration_ms; > uint64_t sb_idl_duration_ms; > uint32_t backoff_ms; > - bool recompute; > }; > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > @@ -24,4 +23,16 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, > void inc_proc_northd_cleanup(void); > bool inc_proc_northd_can_run(struct northd_engine_context *ctx); > > +static inline void > +inc_proc_northd_force_recompute(bool immediate) > +{ > + engine_set_force_recompute(immediate); > +} > + > +static inline bool > +inc_proc_northd_get_force_recompute(void) > +{ > + return engine_get_force_recompute(); > +} > + > #endif /* INC_PROC_NORTHD */ > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index d71114f35..ba3e1ad9d 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -932,7 +932,7 @@ main(int argc, char *argv[]) > if (new_ovnnb_cond_seqno != ovnnb_cond_seqno) { > if (!new_ovnnb_cond_seqno) { > VLOG_INFO("OVN NB IDL reconnected, force recompute."); > - eng_ctx.recompute = true; > + inc_proc_northd_force_recompute(false); > } > ovnnb_cond_seqno = new_ovnnb_cond_seqno; > } > @@ -945,7 +945,7 @@ main(int argc, char *argv[]) > if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) { > if (!new_ovnsb_cond_seqno) { > VLOG_INFO("OVN SB IDL reconnected, force recompute."); > - eng_ctx.recompute = true; > + inc_proc_northd_force_recompute(false); > } > ovnsb_cond_seqno = new_ovnsb_cond_seqno; > } > @@ -969,7 +969,6 @@ main(int argc, char *argv[]) > int64_t loop_start_time = time_wall_msec(); > activity = inc_proc_northd_run(ovnnb_txn, ovnsb_txn, > &eng_ctx); > - eng_ctx.recompute = false; > check_and_add_supported_dhcp_opts_to_sb_db( > ovnsb_txn, ovnsb_idl_loop.idl); > check_and_add_supported_dhcpv6_opts_to_sb_db( > @@ -982,7 +981,7 @@ main(int argc, char *argv[]) > ovnsb_idl_loop.idl, > ovnnb_txn, ovnsb_txn, > &ovnsb_idl_loop); > - } else if (!eng_ctx.recompute) { > + } else if (!inc_proc_northd_get_force_recompute()) { > clear_idl_track = false; > } > > @@ -991,13 +990,13 @@ main(int argc, char *argv[]) > if (!ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop)) { > VLOG_INFO("OVNNB commit failed, " > "force recompute next time."); > - eng_ctx.recompute = true; > + inc_proc_northd_force_recompute(true); > } > > if (!ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop)) { > VLOG_INFO("OVNSB commit failed, " > "force recompute next time."); > - eng_ctx.recompute = true; > + inc_proc_northd_force_recompute(true); > } > run_memory_trimmer(ovnnb_idl_loop.idl, activity); > } else { > @@ -1006,7 +1005,7 @@ main(int argc, char *argv[]) > ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop); > > /* Force a full recompute next time we become active. */ > - eng_ctx.recompute = true; > + inc_proc_northd_force_recompute(false); > } > } else { > /* ovn-northd is paused > @@ -1030,7 +1029,7 @@ main(int argc, char *argv[]) > ovsdb_idl_wait(ovnsb_idl_loop.idl); > > /* Force a full recompute next time we become active. */ > - eng_ctx.recompute = true; > + inc_proc_northd_force_recompute(true); > } > > if (clear_idl_track) { > -- > 2.46.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Oct 16, 2024 at 6:30 PM Numan Siddique <numans@ovn.org> wrote: > On Tue, Sep 24, 2024 at 9:33 AM Ales Musil <amusil@redhat.com> wrote: > > > > There are cases when we need to wake the thread immediately after > > setting force recompute. This is mainly in error handling that > > happens after engine_run() call. In order to achieve that make > > the API more ergonomic with parameters to force the wake if needed. > > > > Reported-at: https://issues.redhat.com/browse/FDP-753 > > Signed-off-by: Ales Musil <amusil@redhat.com> > > The function engine_set_force_recompute(bool) gives the notion that > engine_set_force_recompute(true) -> set the force recompute to true and > engine_set_force_recompute(false) -> set the force recompute to false > > which is the case now (without your patch). > > Changing the meaning of the input param bool to "immediate" doesn't > seem obvious to me. > > Does it make sense to have the below functions instead ? > > void engine_set_force_recompute(void); > void engine_set_force_recompute_immediate(void); > void engine_clear_force_recompute(void); > > > Thanks > Numan > > Hi Numan, thank you for the suggestion, I think it is perfectly reasonable to have more verbose names. I'll send v3 with this naming instead. > > --- > > controller/ovn-controller.c | 17 +++++++---------- > > lib/inc-proc-eng.c | 20 +++++++++++++++++--- > > lib/inc-proc-eng.h | 15 ++++++++++++--- > > northd/inc-proc-northd.c | 14 ++------------ > > northd/inc-proc-northd.h | 13 ++++++++++++- > > northd/ovn-northd.c | 15 +++++++-------- > > 6 files changed, 57 insertions(+), 37 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 168167b1a..f21d46fc3 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -641,7 +641,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(false); > > ovsdb_idl_reset_min_index(ovnsb_idl); > > *reset_ovnsb_idl_min_index = false; > > } > > @@ -4768,7 +4768,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(false); > > } > > version_mismatch = false; > > return true; > > @@ -5382,7 +5382,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(false); > > } > > ovs_cond_seqno = new_ovs_cond_seqno; > > } > > @@ -5399,7 +5399,7 @@ main(int argc, char *argv[]) > > if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) { > > if (!new_ovnsb_cond_seqno) { > > VLOG_INFO("OVNSB IDL reconnected, force recompute."); > > - engine_set_force_recompute(true); > > + engine_set_force_recompute(false); > > } > > ovnsb_cond_seqno = new_ovnsb_cond_seqno; > > } > > @@ -5488,7 +5488,7 @@ main(int argc, char *argv[]) > > br_int_remote.target, > > > br_int_remote.probe_interval)) { > > VLOG_INFO("OVS feature set changed, force recompute."); > > - engine_set_force_recompute(true); > > + engine_set_force_recompute(false); > > > > struct ed_type_lflow_output *lflow_out_data = > > engine_get_internal_data(&en_lflow_output); > > @@ -5510,7 +5510,7 @@ main(int argc, char *argv[]) > > > > VLOG_INFO_RL(&rl, "OVS OpenFlow connection > reconnected," > > "force recompute."); > > - engine_set_force_recompute(true); > > + engine_set_force_recompute(false); > > } > > > > if (chassis && ovs_feature_set_discovered()) { > > @@ -5738,7 +5738,6 @@ main(int argc, char *argv[]) > > VLOG_DBG("engine did not run, force recompute next > time: " > > "br_int %p, chassis %p", br_int, chassis); > > engine_set_force_recompute(true); > > - poll_immediate_wake(); > > } else { > > VLOG_DBG("engine did not run, and it was not needed" > > " either: br_int %p, chassis %p", > > @@ -5748,9 +5747,8 @@ main(int argc, char *argv[]) > > VLOG_DBG("engine was canceled, force recompute next > time: " > > "br_int %p, chassis %p", br_int, chassis); > > engine_set_force_recompute(true); > > - poll_immediate_wake(); > > } else { > > - engine_set_force_recompute(false); > > + engine_clear_force_recompute(); > > } > > > > store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private, > > @@ -6138,7 +6136,6 @@ lflow_cache_flush_cmd(struct unixctl_conn *conn > OVS_UNUSED, > > struct lflow_output_persistent_data *fo_pd = arg_; > > lflow_cache_flush(fo_pd->lflow_cache); > > engine_set_force_recompute(true); > > - poll_immediate_wake(); > > unixctl_command_reply(conn, NULL); > > } > > > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > index a01440bb4..5748727b5 100644 > > --- a/lib/inc-proc-eng.c > > +++ b/lib/inc-proc-eng.c > > @@ -54,9 +54,24 @@ 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(bool immediate) > > { > > - engine_force_recompute = val; > > + engine_force_recompute = true; > > + if (immediate) { > > + poll_immediate_wake(); > > + } > > +} > > + > > +void > > +engine_clear_force_recompute(void) > > +{ > > + engine_force_recompute = false; > > +} > > + > > +bool > > +engine_get_force_recompute(void) > > +{ > > + return engine_force_recompute; > > } > > > > const struct engine_context * > > @@ -556,5 +571,4 @@ engine_trigger_recompute(void) > > { > > VLOG_INFO("User triggered force recompute."); > > engine_set_force_recompute(true); > > - poll_immediate_wake(); > > } > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > index 5bb3b8f3e..db80ae975 100644 > > --- a/lib/inc-proc-eng.h > > +++ b/lib/inc-proc-eng.h > > @@ -297,11 +297,20 @@ void *engine_get_input_data(const char > *input_name, struct engine_node *); > > void engine_add_input(struct engine_node *node, struct engine_node > *input, > > bool (*change_handler)(struct engine_node *, void > *)); > > > > -/* Force the engine to recompute everything if set to true. It is used > > +/* Force the engine to recompute everything. It is used > > * in circumstances when we are not sure there is change or not, or > > * when there is change but the engine couldn't be executed in that > > - * iteration, and the change can't be tracked across iterations */ > > -void engine_set_force_recompute(bool val); > > + * iteration, and the change can't be tracked across iterations. > > + * The immediate makes sure that the loop is woken up immediately > > + * so the next engine run is not delayed. */ > > +void engine_set_force_recompute(bool immediate); > > + > > +/* Clear the force flag for the next run so the engine does the > > + * usual processing without forced full recompute. */ > > +void engine_clear_force_recompute(void); > > + > > +/* Returns whether next engine_run() is forced to rempute. */ > > +bool engine_get_force_recompute(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 > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 1f79916a5..667a13422 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -428,14 +428,6 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn > *ovnnb_txn, > > int64_t start = time_msec(); > > engine_init_run(); > > > > - /* Force a full recompute if instructed to, for example, after a > NB/SB > > - * reconnect event. However, make sure we don't overwrite an > existing > > - * force-recompute request if 'recompute' is false. > > - */ > > - if (ctx->recompute) { > > - engine_set_force_recompute(ctx->recompute); > > - } > > - > > struct engine_context eng_ctx = { > > .ovnnb_idl_txn = ovnnb_txn, > > .ovnsb_idl_txn = ovnsb_txn, > > @@ -448,16 +440,14 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn > *ovnnb_txn, > > if (engine_need_run()) { > > VLOG_DBG("engine did not run, force recompute next time."); > > engine_set_force_recompute(true); > > - poll_immediate_wake(); > > } else { > > VLOG_DBG("engine did not run, and it was not needed"); > > } > > } else if (engine_canceled()) { > > VLOG_DBG("engine was canceled, force recompute next time."); > > engine_set_force_recompute(true); > > - poll_immediate_wake(); > > } else { > > - engine_set_force_recompute(false); > > + engine_clear_force_recompute(); > > } > > > > int64_t now = time_msec(); > > @@ -477,7 +467,7 @@ void inc_proc_northd_cleanup(void) > > bool > > inc_proc_northd_can_run(struct northd_engine_context *ctx) > > { > > - if (ctx->recompute || time_msec() >= ctx->next_run_ms || > > + if (engine_get_force_recompute() || time_msec() >= ctx->next_run_ms > || > > ctx->nb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS || > > ctx->sb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS) { > > return true; > > diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h > > index a2b9b7fdb..7c2cb2e7a 100644 > > --- a/northd/inc-proc-northd.h > > +++ b/northd/inc-proc-northd.h > > @@ -13,7 +13,6 @@ struct northd_engine_context { > > uint64_t nb_idl_duration_ms; > > uint64_t sb_idl_duration_ms; > > uint32_t backoff_ms; > > - bool recompute; > > }; > > > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > @@ -24,4 +23,16 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn > *ovnnb_txn, > > void inc_proc_northd_cleanup(void); > > bool inc_proc_northd_can_run(struct northd_engine_context *ctx); > > > > +static inline void > > +inc_proc_northd_force_recompute(bool immediate) > > +{ > > + engine_set_force_recompute(immediate); > > +} > > + > > +static inline bool > > +inc_proc_northd_get_force_recompute(void) > > +{ > > + return engine_get_force_recompute(); > > +} > > + > > #endif /* INC_PROC_NORTHD */ > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index d71114f35..ba3e1ad9d 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -932,7 +932,7 @@ main(int argc, char *argv[]) > > if (new_ovnnb_cond_seqno != ovnnb_cond_seqno) { > > if (!new_ovnnb_cond_seqno) { > > VLOG_INFO("OVN NB IDL reconnected, force > recompute."); > > - eng_ctx.recompute = true; > > + inc_proc_northd_force_recompute(false); > > } > > ovnnb_cond_seqno = new_ovnnb_cond_seqno; > > } > > @@ -945,7 +945,7 @@ main(int argc, char *argv[]) > > if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) { > > if (!new_ovnsb_cond_seqno) { > > VLOG_INFO("OVN SB IDL reconnected, force > recompute."); > > - eng_ctx.recompute = true; > > + inc_proc_northd_force_recompute(false); > > } > > ovnsb_cond_seqno = new_ovnsb_cond_seqno; > > } > > @@ -969,7 +969,6 @@ main(int argc, char *argv[]) > > int64_t loop_start_time = time_wall_msec(); > > activity = inc_proc_northd_run(ovnnb_txn, ovnsb_txn, > > &eng_ctx); > > - eng_ctx.recompute = false; > > check_and_add_supported_dhcp_opts_to_sb_db( > > ovnsb_txn, ovnsb_idl_loop.idl); > > check_and_add_supported_dhcpv6_opts_to_sb_db( > > @@ -982,7 +981,7 @@ main(int argc, char *argv[]) > > ovnsb_idl_loop.idl, > > ovnnb_txn, ovnsb_txn, > > &ovnsb_idl_loop); > > - } else if (!eng_ctx.recompute) { > > + } else if (!inc_proc_northd_get_force_recompute()) { > > clear_idl_track = false; > > } > > > > @@ -991,13 +990,13 @@ main(int argc, char *argv[]) > > if (!ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop)) { > > VLOG_INFO("OVNNB commit failed, " > > "force recompute next time."); > > - eng_ctx.recompute = true; > > + inc_proc_northd_force_recompute(true); > > } > > > > if (!ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop)) { > > VLOG_INFO("OVNSB commit failed, " > > "force recompute next time."); > > - eng_ctx.recompute = true; > > + inc_proc_northd_force_recompute(true); > > } > > run_memory_trimmer(ovnnb_idl_loop.idl, activity); > > } else { > > @@ -1006,7 +1005,7 @@ main(int argc, char *argv[]) > > ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop); > > > > /* Force a full recompute next time we become active. */ > > - eng_ctx.recompute = true; > > + inc_proc_northd_force_recompute(false); > > } > > } else { > > /* ovn-northd is paused > > @@ -1030,7 +1029,7 @@ main(int argc, char *argv[]) > > ovsdb_idl_wait(ovnsb_idl_loop.idl); > > > > /* Force a full recompute next time we become active. */ > > - eng_ctx.recompute = true; > > + inc_proc_northd_force_recompute(true); > > } > > > > if (clear_idl_track) { > > -- > > 2.46.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > Thanks, Ales
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 168167b1a..f21d46fc3 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -641,7 +641,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(false); ovsdb_idl_reset_min_index(ovnsb_idl); *reset_ovnsb_idl_min_index = false; } @@ -4768,7 +4768,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(false); } version_mismatch = false; return true; @@ -5382,7 +5382,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(false); } ovs_cond_seqno = new_ovs_cond_seqno; } @@ -5399,7 +5399,7 @@ main(int argc, char *argv[]) if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) { if (!new_ovnsb_cond_seqno) { VLOG_INFO("OVNSB IDL reconnected, force recompute."); - engine_set_force_recompute(true); + engine_set_force_recompute(false); } ovnsb_cond_seqno = new_ovnsb_cond_seqno; } @@ -5488,7 +5488,7 @@ main(int argc, char *argv[]) br_int_remote.target, br_int_remote.probe_interval)) { VLOG_INFO("OVS feature set changed, force recompute."); - engine_set_force_recompute(true); + engine_set_force_recompute(false); struct ed_type_lflow_output *lflow_out_data = engine_get_internal_data(&en_lflow_output); @@ -5510,7 +5510,7 @@ main(int argc, char *argv[]) VLOG_INFO_RL(&rl, "OVS OpenFlow connection reconnected," "force recompute."); - engine_set_force_recompute(true); + engine_set_force_recompute(false); } if (chassis && ovs_feature_set_discovered()) { @@ -5738,7 +5738,6 @@ main(int argc, char *argv[]) VLOG_DBG("engine did not run, force recompute next time: " "br_int %p, chassis %p", br_int, chassis); engine_set_force_recompute(true); - poll_immediate_wake(); } else { VLOG_DBG("engine did not run, and it was not needed" " either: br_int %p, chassis %p", @@ -5748,9 +5747,8 @@ main(int argc, char *argv[]) VLOG_DBG("engine was canceled, force recompute next time: " "br_int %p, chassis %p", br_int, chassis); engine_set_force_recompute(true); - poll_immediate_wake(); } else { - engine_set_force_recompute(false); + engine_clear_force_recompute(); } store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private, @@ -6138,7 +6136,6 @@ lflow_cache_flush_cmd(struct unixctl_conn *conn OVS_UNUSED, struct lflow_output_persistent_data *fo_pd = arg_; lflow_cache_flush(fo_pd->lflow_cache); engine_set_force_recompute(true); - poll_immediate_wake(); unixctl_command_reply(conn, NULL); } diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c index a01440bb4..5748727b5 100644 --- a/lib/inc-proc-eng.c +++ b/lib/inc-proc-eng.c @@ -54,9 +54,24 @@ 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(bool immediate) { - engine_force_recompute = val; + engine_force_recompute = true; + if (immediate) { + poll_immediate_wake(); + } +} + +void +engine_clear_force_recompute(void) +{ + engine_force_recompute = false; +} + +bool +engine_get_force_recompute(void) +{ + return engine_force_recompute; } const struct engine_context * @@ -556,5 +571,4 @@ engine_trigger_recompute(void) { VLOG_INFO("User triggered force recompute."); engine_set_force_recompute(true); - poll_immediate_wake(); } diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 5bb3b8f3e..db80ae975 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -297,11 +297,20 @@ void *engine_get_input_data(const char *input_name, struct engine_node *); void engine_add_input(struct engine_node *node, struct engine_node *input, bool (*change_handler)(struct engine_node *, void *)); -/* Force the engine to recompute everything if set to true. It is used +/* Force the engine to recompute everything. It is used * in circumstances when we are not sure there is change or not, or * when there is change but the engine couldn't be executed in that - * iteration, and the change can't be tracked across iterations */ -void engine_set_force_recompute(bool val); + * iteration, and the change can't be tracked across iterations. + * The immediate makes sure that the loop is woken up immediately + * so the next engine run is not delayed. */ +void engine_set_force_recompute(bool immediate); + +/* Clear the force flag for the next run so the engine does the + * usual processing without forced full recompute. */ +void engine_clear_force_recompute(void); + +/* Returns whether next engine_run() is forced to rempute. */ +bool engine_get_force_recompute(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 diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 1f79916a5..667a13422 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -428,14 +428,6 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, int64_t start = time_msec(); engine_init_run(); - /* Force a full recompute if instructed to, for example, after a NB/SB - * reconnect event. However, make sure we don't overwrite an existing - * force-recompute request if 'recompute' is false. - */ - if (ctx->recompute) { - engine_set_force_recompute(ctx->recompute); - } - struct engine_context eng_ctx = { .ovnnb_idl_txn = ovnnb_txn, .ovnsb_idl_txn = ovnsb_txn, @@ -448,16 +440,14 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, if (engine_need_run()) { VLOG_DBG("engine did not run, force recompute next time."); engine_set_force_recompute(true); - poll_immediate_wake(); } else { VLOG_DBG("engine did not run, and it was not needed"); } } else if (engine_canceled()) { VLOG_DBG("engine was canceled, force recompute next time."); engine_set_force_recompute(true); - poll_immediate_wake(); } else { - engine_set_force_recompute(false); + engine_clear_force_recompute(); } int64_t now = time_msec(); @@ -477,7 +467,7 @@ void inc_proc_northd_cleanup(void) bool inc_proc_northd_can_run(struct northd_engine_context *ctx) { - if (ctx->recompute || time_msec() >= ctx->next_run_ms || + if (engine_get_force_recompute() || time_msec() >= ctx->next_run_ms || ctx->nb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS || ctx->sb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS) { return true; diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h index a2b9b7fdb..7c2cb2e7a 100644 --- a/northd/inc-proc-northd.h +++ b/northd/inc-proc-northd.h @@ -13,7 +13,6 @@ struct northd_engine_context { uint64_t nb_idl_duration_ms; uint64_t sb_idl_duration_ms; uint32_t backoff_ms; - bool recompute; }; void inc_proc_northd_init(struct ovsdb_idl_loop *nb, @@ -24,4 +23,16 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, void inc_proc_northd_cleanup(void); bool inc_proc_northd_can_run(struct northd_engine_context *ctx); +static inline void +inc_proc_northd_force_recompute(bool immediate) +{ + engine_set_force_recompute(immediate); +} + +static inline bool +inc_proc_northd_get_force_recompute(void) +{ + return engine_get_force_recompute(); +} + #endif /* INC_PROC_NORTHD */ diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index d71114f35..ba3e1ad9d 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -932,7 +932,7 @@ main(int argc, char *argv[]) if (new_ovnnb_cond_seqno != ovnnb_cond_seqno) { if (!new_ovnnb_cond_seqno) { VLOG_INFO("OVN NB IDL reconnected, force recompute."); - eng_ctx.recompute = true; + inc_proc_northd_force_recompute(false); } ovnnb_cond_seqno = new_ovnnb_cond_seqno; } @@ -945,7 +945,7 @@ main(int argc, char *argv[]) if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) { if (!new_ovnsb_cond_seqno) { VLOG_INFO("OVN SB IDL reconnected, force recompute."); - eng_ctx.recompute = true; + inc_proc_northd_force_recompute(false); } ovnsb_cond_seqno = new_ovnsb_cond_seqno; } @@ -969,7 +969,6 @@ main(int argc, char *argv[]) int64_t loop_start_time = time_wall_msec(); activity = inc_proc_northd_run(ovnnb_txn, ovnsb_txn, &eng_ctx); - eng_ctx.recompute = false; check_and_add_supported_dhcp_opts_to_sb_db( ovnsb_txn, ovnsb_idl_loop.idl); check_and_add_supported_dhcpv6_opts_to_sb_db( @@ -982,7 +981,7 @@ main(int argc, char *argv[]) ovnsb_idl_loop.idl, ovnnb_txn, ovnsb_txn, &ovnsb_idl_loop); - } else if (!eng_ctx.recompute) { + } else if (!inc_proc_northd_get_force_recompute()) { clear_idl_track = false; } @@ -991,13 +990,13 @@ main(int argc, char *argv[]) if (!ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop)) { VLOG_INFO("OVNNB commit failed, " "force recompute next time."); - eng_ctx.recompute = true; + inc_proc_northd_force_recompute(true); } if (!ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop)) { VLOG_INFO("OVNSB commit failed, " "force recompute next time."); - eng_ctx.recompute = true; + inc_proc_northd_force_recompute(true); } run_memory_trimmer(ovnnb_idl_loop.idl, activity); } else { @@ -1006,7 +1005,7 @@ main(int argc, char *argv[]) ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop); /* Force a full recompute next time we become active. */ - eng_ctx.recompute = true; + inc_proc_northd_force_recompute(false); } } else { /* ovn-northd is paused @@ -1030,7 +1029,7 @@ main(int argc, char *argv[]) ovsdb_idl_wait(ovnsb_idl_loop.idl); /* Force a full recompute next time we become active. */ - eng_ctx.recompute = true; + inc_proc_northd_force_recompute(true); } if (clear_idl_track) {
There are cases when we need to wake the thread immediately after setting force recompute. This is mainly in error handling that happens after engine_run() call. In order to achieve that make the API more ergonomic with parameters to force the wake if needed. Reported-at: https://issues.redhat.com/browse/FDP-753 Signed-off-by: Ales Musil <amusil@redhat.com> --- controller/ovn-controller.c | 17 +++++++---------- lib/inc-proc-eng.c | 20 +++++++++++++++++--- lib/inc-proc-eng.h | 15 ++++++++++++--- northd/inc-proc-northd.c | 14 ++------------ northd/inc-proc-northd.h | 13 ++++++++++++- northd/ovn-northd.c | 15 +++++++-------- 6 files changed, 57 insertions(+), 37 deletions(-)