On Sat, Dec 7, 2019 at 4:53 PM Han Zhou <hz...@ovn.org> wrote: > > > > On Sat, Dec 7, 2019 at 3:21 AM Dumitru Ceara <dce...@redhat.com> wrote: > > > > On Sat, Dec 7, 2019 at 1:36 AM Han Zhou <hz...@ovn.org> wrote: > > > > > > Thanks Dumitru for the patch. Please see my comments below. > > > > > > > Hi Han, > > > > Thanks for the review. > > > > > On Fri, Dec 6, 2019 at 5:38 AM Dumitru Ceara <dce...@redhat.com> wrote: > > > > > > > > If the last ovn-controller main loop run started a transaction to the > > > > Southbound DB and the transaction is still in progress, ovn-controller > > > > will not call engine_run(). In case there were changes to the DB, > > > > engine_need_run() would return true which would trigger an immediate > > > > forced recompute in the next processing loop iteration. > > > > > > > > However, there are scenarios when updates can be processed incrementally > > > > even if no Southbound DB transaction is available. One example, often > > > > seen in the field, is when the MAC_Binding table is updated. Currently > > > > such an update received while a transaction is still committing would > > > > trigger a forced recompute. > > > > > > > > To minimize the number of forced recomputes, we now call > > > > engine_run(false), i.e., try to process updates incrementally without > > > > allowing recompute, also when ovnsb_idl_txn == NULL. This is safe > > > > because ovnsb_idl_txn is not used by change_handlers and run handlers > > > > are not allowed to execute when calling engine_run(false). > > > > > > > This patch assumes we will never need to do SB updates when handling > > > changes incrementally. There are two problems: > > > 1. I don't see a reason why a change_handler (for doing incremental > > > compute) can't update SB DB. For example Numan mentioned about working on > > > incremental processing for port-bindings on local chassis. I believe > > > those change handling would trigger SB updates, e.g. claiming/releasing a > > > port. > > > 2. Even if we can keep this assumption and never do incremental > > > processing for changes that require updating SB DB, how do we ensure this > > > constraint is enforced in coding? > > > > > > > I think my commit message is not clear enough. I didn't mean that > > change handlers will never perform SB updates. What I meant was that > > if a change handler needs to perform SB updates it needs to validate > > that ovnsb_idl_txn != NULL and if it's NULL return "false" to trigger > > a recompute. At this moment we have no change handler that updates the > > SB database but as you pointed out we might have them in the future. > > Then, the new change handlers will have to make sure that > > ovnsb_idl_txn is not NULL. > > > > OK, maybe I missed the point. Now let me rephrase your idea. > This patch is to do only incremental processing when txn is NULL, which means > executing change handlers instead of run() methods. Assume some change > handlers need to update SB DB, some don't. > If the incoming change (when txn is NULL) doesn't trigger SB update, then > change handlers can complete without aborting. - this seems to happen in most > cases. > If the incoming change happen to require another SB update, since txn is > NULL, the change handlers that are supposed to do the update will need to > return false and result in aborting. - this may not happen very often, so the > approach of this patch should be effective.
Exactly. > > > > For 2), it may be easy. We could hide the interface get_context(), and > > > always pass the context to xxx_run() interface, so that ovnsb_idl_txn can > > > be accessed by recompute but never by change_handlers. > > > What I am really concerned is 1). I think the long term solution is to > > > make the OVSDB change tracking available across iterations. Maybe it is > > > not a trivial work. > > > > > > > As mentioned above, I don't think we need this as long as the contract > > of get_context() is clear and the users know to check for NULL txn > > pointers in change handlers. > > > Agree, but I'd suggest to document the patter more clearly, that a run() > handler never expects NULL txn, and a change handler need to check if txn is > NULL before using it. If it is NULL, it should return false (can't process > the change). I'll send a v2 soon and document this requirement better. Thanks, Dumitru > > > > If we believe there is a big performance gain here and we are sure we > > > don't need to update SB DB in change handlers (in short term), we may > > > proceed with this approach as long as 2) is addressed, and revert this > > > whenever the long term solution (OVSDB change tracking over iterations) > > > is ready. > > > > There's no real reason to not try to process changes incrementally > > even when a SB txn is in progress (ovnsb_idl_txn == NULL). We use the > > same reasoning for !ofctrl_can_put() in order to avoid forced > > recomputes. We saw it on customer deployments that MAC_Binding updates > > received while a SB transaction was still in progress were triggering > > unnecessary forced full recomputes that last in the order of tens of > > seconds. > > > > What do you think? > > > Ack. > > > > > > > > To make sure that other users of ovnsb_idl_txn, like pinctrl_run(), > > > > get a chance to run as soon as the transaction is completed, if the > > > > engine has successfully run and ovnsb_idl_txn is NULL we trigger an > > > > immediate wake and a new iteration of the processing loop. > > > > > > I think this will cause busy loop until the transaction completes. And I > > > think it is not necessary because when transaction completes there will > > > be a jsonrpc message coming and trigger the main loop, and pinctrl will > > > get a chance to run. > > > > Ah, right, I missed that case. I can just remove this check. > > > > Thanks, > > Dumitru > > > > > > > > > > > > > CC: Han Zhou <hz...@ovn.org> > > > > CC: Numan Siddique <num...@ovn.org> > > > > Signed-off-by: Dumitru Ceara <dce...@redhat.com> > > > > --- > > > > controller/ovn-controller.c | 13 +++++++++++++ > > > > lib/inc-proc-eng.h | 8 +++++++- > > > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > > index 5874776..4a27d5f 100644 > > > > --- a/controller/ovn-controller.c > > > > +++ b/controller/ovn-controller.c > > > > @@ -2100,6 +2100,14 @@ main(int argc, char *argv[]) > > > > } else { > > > > engine_run(true); > > > > } > > > > + } else { > > > > + /* Even if there's no SB DB transaction > > > > available, > > > > + * try to run the engine so that we can handle > > > > any > > > > + * incremental changes that don't require a > > > > recompute. > > > > + * If a recompute is required, the engine will > > > > abort, > > > > + * triggerring a full run in the next > > > > iteration. > > > > + */ > > > > + engine_run(false); > > > > } > > > > stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, > > > > time_msec()); > > > > @@ -2168,6 +2176,11 @@ main(int argc, char *argv[]) > > > > "br_int %p, chassis %p", br_int, chassis); > > > > engine_set_force_recompute(true); > > > > poll_immediate_wake(); > > > > + } else if (!ovnsb_idl_txn) { > > > > + VLOG_DBG("engine ran, no SB DB transaction available, " > > > > + "trigger an immediate loop run: " > > > > + "br_int %p, chassis %p", br_int, chassis); > > > > + poll_immediate_wake(); > > > > } else { > > > > engine_set_force_recompute(false); > > > > } > > > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > > > index 5b92971..2f90b0a 100644 > > > > --- a/lib/inc-proc-eng.h > > > > +++ b/lib/inc-proc-eng.h > > > > @@ -189,7 +189,13 @@ 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); > > > > > > > > -const struct engine_context * engine_get_context(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 > > > > + * iteration. > > > > + * Therefore, it is the responsibility of the caller to check the > > > > context > > > > + * values when called from change_handlers. > > > > + */ > > > > +const struct engine_context *engine_get_context(void); > > > > > > > > void engine_set_context(const struct engine_context *); > > > > > > > > -- > > > > 1.8.3.1 > > > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev