Thanks Mark and Numan for the review. Ben, now this is merged to OVN repo. Could you help backporting it to 2.12?
Thanks, Han On Thu, Jul 25, 2019 at 1:23 PM Mark Michelson <mmich...@redhat.com> wrote: > > Forgot the e-mail address after my name :) > > Acked-by: Mark Michelson <mmich...@redhat.com> > > On 7/25/19 4:16 PM, Mark Michelson wrote: > > Acked-by: Mark Michelson > > > > Thanks for the informative commit message! > > > > On 7/23/19 10:52 PM, Han Zhou wrote: > >> From: Han Zhou <hzh...@ebay.com> > >> > >> When there are in-flight flow-installations pending to ovs-vswitchd, > >> current incremental processing logic prioritizes new change handling. > >> However, in scenarios where frequent recomputes are triggered, the > >> later recompute would block the flow-installation for previously > >> computed flows because recompute usually takes long time, especially > >> when there are large number of flows. This results in worse latency > >> than the version without incremental processing in specific scale > >> test scenarios. > >> > >> While we can simply fix the problem by prioritizing flow installation > >> rather than new change handling, it can cause the incremental > >> processing to degrade to always recompute in certain scenarios when > >> there are some changes triggering recomputes, followed by a lot of > >> continously coming changes that can be handled incrementally. Because > >> OVSDB change tracker cannot preserve changes across iterations, once > >> the recompute is triggered and resulted in a lot of pending messages > >> to ovs-vswitchd, and if we choose to skip the engine_run() > >> in the next iteration when a incrementally processible change comes, > >> we miss the opportunity to handle that tracked change and will have > >> to trigger recompute again in the next next iteration, and so on, if > >> such changes come continously. > >> > >> This patch solves the problem by introducing > >> engine_set_abort_recompute(), > >> so that we can prioritize new change handling if the change can be > >> incrementally processed, but if the change triggers recompute, we > >> abort there without spending CPU on the recompute to avoid blocking > >> the previous computed flow installation. > >> > >> Reported-by: Daniel Alvarez Sanchez <dalva...@redhat.com> > >> Reported-by: Numan Siddique <nusid...@redhat.com> > >> Reported-at: > >> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html > >> Tested-by: Numan Siddique <nusid...@redhat.com> > >> Signed-off-by: Han Zhou <hzh...@ebay.com> > >> --- > >> ovn/controller/ofctrl.c | 2 +- > >> ovn/controller/ofctrl.h | 1 + > >> ovn/controller/ovn-controller.c | 30 +++++++++++++++++++++++++++--- > >> ovn/lib/inc-proc-eng.c | 26 ++++++++++++++++++++++---- > >> ovn/lib/inc-proc-eng.h | 9 +++++++-- > >> 5 files changed, 58 insertions(+), 10 deletions(-) > >> > >> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c > >> index 043abd6..0fcaa72 100644 > >> --- a/ovn/controller/ofctrl.c > >> +++ b/ovn/controller/ofctrl.c > >> @@ -985,7 +985,7 @@ add_meter(struct ovn_extend_table_info *m_desired, > >> * in the correct state and not backlogged with existing flow_mods. > >> (Our > >> * criteria for being backlogged appear very conservative, but the > >> socket > >> * between ovn-controller and OVS provides some buffering.) */ > >> -static bool > >> +bool > >> ofctrl_can_put(void) > >> { > >> if (state != S_UPDATE_FLOWS > >> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h > >> index ed8918a..2b21c11 100644 > >> --- a/ovn/controller/ofctrl.h > >> +++ b/ovn/controller/ofctrl.h > >> @@ -51,6 +51,7 @@ void ofctrl_put(struct ovn_desired_flow_table *, > >> const struct sbrec_meter_table *, > >> int64_t nb_cfg, > >> bool flow_changed); > >> +bool ofctrl_can_put(void); > >> void ofctrl_wait(void); > >> void ofctrl_destroy(void); > >> int64_t ofctrl_get_cur_cfg(void); > >> diff --git a/ovn/controller/ovn-controller.c > >> b/ovn/controller/ovn-controller.c > >> index c4883aa..866cc1c 100644 > >> --- a/ovn/controller/ovn-controller.c > >> +++ b/ovn/controller/ovn-controller.c > >> @@ -1869,6 +1869,7 @@ main(int argc, char *argv[]) > >> uint64_t engine_run_id = 0; > >> uint64_t old_engine_run_id = 0; > >> + bool engine_aborted = false; > >> unsigned int ovs_cond_seqno = UINT_MAX; > >> unsigned int ovnsb_cond_seqno = UINT_MAX; > >> @@ -1955,7 +1956,30 @@ main(int argc, char *argv[]) > >> stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME, > >> time_msec()); > >> if (ovnsb_idl_txn) { > >> - engine_run(&en_flow_output, ++engine_run_id); > >> + if (!ofctrl_can_put()) { > >> + /* When there are in-flight messages > >> pending to > >> + * ovs-vswitchd, we should hold on > >> recomputing so > >> + * that the previous flow installations > >> won't be > >> + * delayed. However, we still want to > >> try if > >> + * recompute is not needed and we can > >> quickly > >> + * incrementally process the new changes, > >> to avoid > >> + * unnecessarily forced recomputes later > >> on. This > >> + * is because the OVSDB change tracker > >> cannot > >> + * preserve tracked changes across > >> iterations. If > >> + * change tracking is improved, we can > >> simply skip > >> + * this round of engine_run and continue > >> processing > >> + * acculated changes incrementally later > >> when > >> + * ofctrl_can_put() returns true. */ > >> + if (!engine_aborted) { > >> + engine_set_abort_recompute(true); > >> + engine_aborted = > >> engine_run(&en_flow_output, > >> + > >> ++engine_run_id); > >> + } > >> + } else { > >> + engine_set_abort_recompute(false); > >> + engine_aborted = false; > >> + engine_run(&en_flow_output, > >> ++engine_run_id); > >> + } > >> } > >> stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, > >> time_msec()); > >> @@ -1993,8 +2017,8 @@ main(int argc, char *argv[]) > >> } > >> } > >> - if (old_engine_run_id == engine_run_id) { > >> - if (engine_need_run(&en_flow_output)) { > >> + if (old_engine_run_id == engine_run_id || engine_aborted) { > >> + if (engine_aborted || > >> engine_need_run(&en_flow_output)) { > >> VLOG_DBG("engine did not run, force recompute > >> next time: " > >> "br_int %p, chassis %p", br_int, chassis); > >> engine_set_force_recompute(true); > >> diff --git a/ovn/lib/inc-proc-eng.c b/ovn/lib/inc-proc-eng.c > >> index 1ddea1a..1064a08 100644 > >> --- a/ovn/lib/inc-proc-eng.c > >> +++ b/ovn/lib/inc-proc-eng.c > >> @@ -31,6 +31,7 @@ > >> VLOG_DEFINE_THIS_MODULE(inc_proc_eng); > >> static bool engine_force_recompute = false; > >> +static bool engine_abort_recompute = false; > >> static const struct engine_context *engine_context; > >> void > >> @@ -39,6 +40,12 @@ engine_set_force_recompute(bool val) > >> engine_force_recompute = val; > >> } > >> +void > >> +engine_set_abort_recompute(bool val) > >> +{ > >> + engine_abort_recompute = val; > >> +} > >> + > >> const struct engine_context * > >> engine_get_context(void) > >> { > >> @@ -121,11 +128,11 @@ engine_ovsdb_node_add_index(struct engine_node > >> *node, const char *name, > >> ed->n_indexes ++; > >> } > >> -void > >> +bool > >> engine_run(struct engine_node *node, uint64_t run_id) > >> { > >> if (node->run_id == run_id) { > >> - return; > >> + return true; > >> } > >> node->run_id = run_id; > >> @@ -133,11 +140,13 @@ engine_run(struct engine_node *node, uint64_t > >> run_id) > >> if (!node->n_inputs) { > >> node->run(node); > >> VLOG_DBG("node: %s, changed: %d", node->name, node->changed); > >> - return; > >> + return true; > >> } > >> for (size_t i = 0; i < node->n_inputs; i++) { > >> - engine_run(node->inputs[i].node, run_id); > >> + if (!engine_run(node->inputs[i].node, run_id)) { > >> + return false; > >> + } > >> } > >> bool need_compute = false; > >> @@ -160,6 +169,10 @@ engine_run(struct engine_node *node, uint64_t > >> run_id) > >> if (need_recompute) { > >> VLOG_DBG("node: %s, recompute (%s)", node->name, > >> engine_force_recompute ? "forced" : "triggered"); > >> + if (engine_abort_recompute) { > >> + VLOG_DBG("node: %s, recompute aborted", node->name); > >> + return false; > >> + } > >> node->run(node); > >> } else if (need_compute) { > >> for (size_t i = 0; i < node->n_inputs; i++) { > >> @@ -170,6 +183,10 @@ engine_run(struct engine_node *node, uint64_t > >> run_id) > >> VLOG_DBG("node: %s, can't handle change for > >> input %s, " > >> "fall back to recompute", > >> node->name, node->inputs[i].node->name); > >> + if (engine_abort_recompute) { > >> + VLOG_DBG("node: %s, recompute aborted", > >> node->name); > >> + return false; > >> + } > >> node->run(node); > >> break; > >> } > >> @@ -178,6 +195,7 @@ engine_run(struct engine_node *node, uint64_t run_id) > >> } > >> VLOG_DBG("node: %s, changed: %d", node->name, node->changed); > >> + return true; > >> } > >> bool > >> diff --git a/ovn/lib/inc-proc-eng.h b/ovn/lib/inc-proc-eng.h > >> index aab899e..c3d7b5e 100644 > >> --- a/ovn/lib/inc-proc-eng.h > >> +++ b/ovn/lib/inc-proc-eng.h > >> @@ -121,8 +121,9 @@ struct engine_node { > >> void engine_init(struct engine_node *); > >> /* Execute the processing recursively, which should be called in the > >> main > >> - * loop. */ > >> -void engine_run(struct engine_node *, uint64_t run_id); > >> + * loop. Returns true if the execution is compelte, false if it is > >> aborted, > >> + * which could happen when engine_abort_recompute is set. */ > >> +bool engine_run(struct engine_node *, uint64_t run_id); > >> /* Clean up the data for the engine nodes recursively. It calls each > >> node's > >> * cleanup() method if not NULL. It should be called before the program > >> @@ -150,6 +151,10 @@ void engine_add_input(struct engine_node *node, > >> struct engine_node *input, > >> * iteration, and the change can't be tracked across iterations */ > >> void engine_set_force_recompute(bool val); > >> +/* Set the flag to cause engine execution to be aborted when there > >> + * is any recompute to be triggered in any node. */ > >> +void engine_set_abort_recompute(bool val); > >> + > >> const struct engine_context * engine_get_context(void); > >> void engine_set_context(const struct engine_context *); > >> > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
