On Fri, Jun 5, 2020 at 12:39 AM Han Zhou <[email protected]> wrote: > On Thu, Jun 4, 2020 at 11:47 AM <[email protected]> wrote: > > > > From: Numan Siddique <[email protected]> > > > > The function ovsdb_idl_loop_run(), after calling ovsdb_idl_run(), > > returns a transaction object (of type 'struct ovsdb_idl_txn'). > > The returned transaction object can be NULL if there is a pending > > transaction (loop->committing_txn) in the idl loop object. > > > > Normally the clients of idl library, first call ovsdb_idl_loop_run(), > > then do their own processing and create any idl transactions during > > this processing and then finally call ovsdb_idl_loop_commit_and_wait(). > > > > If ovsdb_idl_loop_run() returns NULL transaction object, then much > > of the processing done by the client gets wasted as in the case > > of ovn-controller. > > > > The client (in this case ovn-controller), can skip the processing > > and instead call ovsdb_idl_loop_commit_and_wait() if the transaction > > oject is NULL. But ovn-controller uses IDL tracking and it may > > loose the tracked changes in that run. > > > > This patch tries to improve this scenario, by checking if the > > pending transaction can be committed in the ovsdb_idl_loop_run() > > itself and if the pending transaction is cleared (because of the > > response messages from ovsdb-server due to a transaction message > > in the previous run), ovsdb_idl_loop_run() can return a valid > > transaction object. > > > > CC: Han Zhou <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > lib/ovsdb-idl.c | 134 +++++++++++++++++++++++++++++++----------------- > > 1 file changed, 88 insertions(+), 46 deletions(-) > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index f54e360e3..b436b3b80 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -385,6 +385,8 @@ static void ovsdb_idl_send_cond_change(struct > ovsdb_idl *idl); > > static void ovsdb_idl_destroy_indexes(struct ovsdb_idl_table *); > > static void ovsdb_idl_add_to_indexes(const struct ovsdb_idl_row *); > > static void ovsdb_idl_remove_from_indexes(const struct ovsdb_idl_row *); > > +static enum ovsdb_idl_txn_status ovsdb_idl_try_commit_loop_txn( > > + struct ovsdb_idl_loop *loop); > > > > static void > > ovsdb_idl_db_init(struct ovsdb_idl_db *db, const struct ovsdb_idl_class > *class, > > @@ -5340,6 +5342,12 @@ struct ovsdb_idl_txn * > > ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop) > > { > > ovsdb_idl_run(loop->idl); > > + > > + /* See if we can commit the loop->committing_txn. */ > > + if (loop->committing_txn) { > > + ovsdb_idl_try_commit_loop_txn(loop); > > + } > > + > > loop->open_txn = (loop->committing_txn > > || ovsdb_idl_get_seqno(loop->idl) == > loop->skip_seqno > > ? NULL > > @@ -5347,6 +5355,51 @@ ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop) > > return loop->open_txn; > > } > > > > +/* Attempts to commit the current idl loop transaction and destroys the > > + * transaction if not TXN_INCOMPLETE. */ > > +static enum ovsdb_idl_txn_status > > +ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop) > > +{ > > + ovs_assert(loop->committing_txn); > > + > > + struct ovsdb_idl_txn *txn = loop->committing_txn; > > + enum ovsdb_idl_txn_status status = ovsdb_idl_txn_commit(txn); > > + if (status != TXN_INCOMPLETE) { > > + switch (status) { > > + case TXN_TRY_AGAIN: > > + /* We want to re-evaluate the database when it's changed > from > > + * the contents that it had when we started the commit. > (That > > + * might have already happened.) */ > > + loop->skip_seqno = loop->precommit_seqno; > > + break; > > + > > + case TXN_SUCCESS: > > + /* Possibly some work on the database was deferred because > no > > + * further transaction could proceed. */ > > + loop->cur_cfg = loop->next_cfg; > > + break; > > + > > + case TXN_UNCHANGED: > > + loop->cur_cfg = loop->next_cfg; > > + break; > > + > > + case TXN_ABORTED: > > + case TXN_NOT_LOCKED: > > + case TXN_ERROR: > > + break; > > + > > + case TXN_UNCOMMITTED: > > + case TXN_INCOMPLETE: > > + default: > > + OVS_NOT_REACHED(); > > + } > > + ovsdb_idl_txn_destroy(txn); > > + loop->committing_txn = NULL; > > + } > > + > > + return status; > > +} > > + > > /* Attempts to commit the current transaction, if one is open, and sets > up the > > * poll loop to wake up when some more work might be needed. > > * > > @@ -5377,57 +5430,46 @@ ovsdb_idl_loop_commit_and_wait(struct > ovsdb_idl_loop *loop) > > loop->precommit_seqno = ovsdb_idl_get_seqno(loop->idl); > > } > > > > - struct ovsdb_idl_txn *txn = loop->committing_txn; > > - int retval; > > - if (txn) { > > - enum ovsdb_idl_txn_status status = ovsdb_idl_txn_commit(txn); > > - if (status != TXN_INCOMPLETE) { > > - switch (status) { > > - case TXN_TRY_AGAIN: > > - /* We want to re-evaluate the database when it's changed > from > > - * the contents that it had when we started the commit. > (That > > - * might have already happened.) */ > > - loop->skip_seqno = loop->precommit_seqno; > > - if (ovsdb_idl_get_seqno(loop->idl) != loop->skip_seqno) > { > > - poll_immediate_wake(); > > - } > > - retval = 0; > > - break; > > - > > - case TXN_SUCCESS: > > - /* Possibly some work on the database was deferred > because no > > - * further transaction could proceed. Wake up again. */ > > - retval = 1; > > - loop->cur_cfg = loop->next_cfg; > > + int retval = -1; > > + if (loop->committing_txn) { > > + enum ovsdb_idl_txn_status status = > ovsdb_idl_try_commit_loop_txn(loop); > > + switch (status) { > > + case TXN_TRY_AGAIN: > > + /* We want to re-evaluate the database when it's changed > from > > + * the contents that it had when we started the commit. > (That > > + * might have already happened.) */ > > + if (ovsdb_idl_get_seqno(loop->idl) != loop->skip_seqno) { > > poll_immediate_wake(); > > - break; > > - > > - case TXN_UNCHANGED: > > - retval = 1; > > - loop->cur_cfg = loop->next_cfg; > > - break; > > - > > - case TXN_ABORTED: > > - case TXN_NOT_LOCKED: > > - case TXN_ERROR: > > - retval = 0; > > - break; > > - > > - case TXN_UNCOMMITTED: > > - case TXN_INCOMPLETE: > > - default: > > - OVS_NOT_REACHED(); > > } > > - ovsdb_idl_txn_destroy(txn); > > - loop->committing_txn = NULL; > > - } else { > > + retval = 0; > > + break; > > + > > + case TXN_SUCCESS: > > + /* Possibly some work on the database was deferred because > no > > + * further transaction could proceed. Wake up again. */ > > + retval = 1; > > + poll_immediate_wake(); > > + break; > > + > > + case TXN_UNCHANGED: > > + retval = 1; > > + break; > > + > > + case TXN_ABORTED: > > + case TXN_NOT_LOCKED: > > + case TXN_ERROR: > > + retval = 0; > > + break; > > + > > + case TXN_INCOMPLETE: > > retval = -1; > > + break; > > + > > + case TXN_UNCOMMITTED: > > + default: > > + OVS_NOT_REACHED(); > > } > > - } else { > > - /* Not a meaningful return value: no transaction was in > progress. */ > > - retval = 1; > > } > > - > > ovsdb_idl_wait(loop->idl); > > > > return retval; > > -- > > 2.26.2 > > > > Thanks for the v2. It seems a lot of redundant code for the switch-case. > Would it be better to update the ovsdb_idl_try_commit_loop_txn(struct > ovsdb_idl_loop *loop) signature to: > > static int > ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop, bool > *may_need_wakeup) > > In ovsdb_idl_loop_run() it calls with > ovsdb_idl_try_commit_loop_txn(loop, NULL); > > In ovsdb_idl_loop_commit_and_wait() it calls with > ovsdb_idl_try_commit_loop_txn(loop, &need_wakeup); > if (need_wakeup) { > poll_immediate_wake(); > } > > What do you think? >
This makes more sense. I submitted v3. Thanks Numan > > Thanks, > Han > _______________________________________________ > 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
