Message ID | 20200604184715.168340-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,ovs,v2] ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run. | expand |
On Thu, Jun 4, 2020 at 11:47 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > 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 <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > 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? Thanks, Han
On Fri, Jun 5, 2020 at 12:39 AM Han Zhou <hzhou@ovn.org> wrote: > On Thu, Jun 4, 2020 at 11:47 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > 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 <hzhou@ovn.org> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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;