On Thu, Oct 17, 2024 at 2:32 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]>
> ---
> v3: Rebase on top of latest main.
> Address comment from Numan, make the function names more explicit.
Thanks. Applied to main.
Numan
> ---
> controller/ovn-controller.c | 25 +++++++++++--------------
> lib/inc-proc-eng.c | 26 ++++++++++++++++++++++----
> lib/inc-proc-eng.h | 17 ++++++++++++++---
> northd/inc-proc-northd.c | 18 ++++--------------
> northd/inc-proc-northd.h | 19 ++++++++++++++++++-
> northd/ovn-northd.c | 15 +++++++--------
> 6 files changed, 76 insertions(+), 44 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index e3266792b..c40fb3d43 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();
> ovsdb_idl_reset_min_index(ovnsb_idl);
> *reset_ovnsb_idl_min_index = false;
> }
> @@ -4826,7 +4826,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();
> }
> version_mismatch = false;
> return true;
> @@ -5454,7 +5454,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();
> }
> ovs_cond_seqno = new_ovs_cond_seqno;
> }
> @@ -5471,7 +5471,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();
> }
> ovnsb_cond_seqno = new_ovnsb_cond_seqno;
> }
> @@ -5560,7 +5560,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();
>
> struct ed_type_lflow_output *lflow_out_data =
> engine_get_internal_data(&en_lflow_output);
> @@ -5582,7 +5582,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();
> }
>
> if (chassis && ovs_feature_set_discovered()) {
> @@ -5817,8 +5817,7 @@ main(int argc, char *argv[])
> if (engine_need_run()) {
> 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();
> + engine_set_force_recompute_immediate();
> } else {
> VLOG_DBG("engine did not run, and it was not needed"
> " either: br_int %p, chassis %p",
> @@ -5827,10 +5826,9 @@ main(int argc, char *argv[])
> } else if (engine_canceled()) {
> 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();
> + engine_set_force_recompute_immediate();
> } else {
> - engine_set_force_recompute(false);
> + engine_clear_force_recompute();
> }
>
> store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private,
> @@ -5883,7 +5881,7 @@ main(int argc, char *argv[])
>
> if (!ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop)) {
> VLOG_INFO("OVNSB commit failed, force recompute next time.");
> - engine_set_force_recompute(true);
> + engine_set_force_recompute_immediate();
> }
>
> int ovs_txn_status = ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
> @@ -6232,8 +6230,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);
> - poll_immediate_wake();
> + engine_set_force_recompute_immediate();
> unixctl_command_reply(conn, NULL);
> }
>
> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> index a01440bb4..56dc62c4f 100644
> --- a/lib/inc-proc-eng.c
> +++ b/lib/inc-proc-eng.c
> @@ -54,9 +54,28 @@ 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(void)
> {
> - engine_force_recompute = val;
> + engine_force_recompute = true;
> +}
> +
> +void
> +engine_set_force_recompute_immediate(void)
> +{
> + engine_force_recompute = true;
> + 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 *
> @@ -555,6 +574,5 @@ void
> engine_trigger_recompute(void)
> {
> VLOG_INFO("User triggered force recompute.");
> - engine_set_force_recompute(true);
> - poll_immediate_wake();
> + engine_set_force_recompute_immediate();
> }
> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> index 5bb3b8f3e..12e232020 100644
> --- a/lib/inc-proc-eng.h
> +++ b/lib/inc-proc-eng.h
> @@ -297,11 +297,22 @@ 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. */
> +void engine_set_force_recompute(void);
> +
> +/* Same as "engine_set_force_recompute()", but the poll_loop is woken up
> + * immediately and the next engine run is not delayed. */
> +void engine_set_force_recompute_immediate(void);
> +
> +/* 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..8c834facb 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,
> @@ -447,17 +439,15 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn
> *ovnnb_txn,
> if (!engine_has_run()) {
> if (engine_need_run()) {
> VLOG_DBG("engine did not run, force recompute next time.");
> - engine_set_force_recompute(true);
> - poll_immediate_wake();
> + engine_set_force_recompute_immediate();
> } 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();
> + engine_set_force_recompute_immediate();
> } 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..0f763d8df 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,22 @@ 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(void)
> +{
> + engine_set_force_recompute();
> +}
> +
> +static inline void
> +inc_proc_northd_force_recompute_immediate(void)
> +{
> + 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 89ef4e870..fb29c3c21 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -968,7 +968,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();
> }
> ovnnb_cond_seqno = new_ovnnb_cond_seqno;
> }
> @@ -981,7 +981,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();
> }
> ovnsb_cond_seqno = new_ovnsb_cond_seqno;
> }
> @@ -1005,7 +1005,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(
> @@ -1018,7 +1017,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;
> }
>
> @@ -1027,13 +1026,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_immediate();
> }
>
> 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_immediate();
> }
> run_memory_trimmer(ovnnb_idl_loop.idl, activity);
> } else {
> @@ -1042,7 +1041,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();
> }
> } else {
> /* ovn-northd is paused
> @@ -1066,7 +1065,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_immediate();
> }
>
> if (clear_idl_track) {
> --
> 2.46.2
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
