On Wed, Oct 16, 2024 at 6:30 PM Numan Siddique <[email protected]> wrote:
> On Tue, Sep 24, 2024 at 9:33 AM Ales Musil <[email protected]> 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 <[email protected]> > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
