| Message ID | 20200603165139.1107659-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovs] ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run. | expand |
On Wed, Jun 3, 2020 at 9:51 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. Thanks Numan for addressing this. I think the patch is to trying to make the TXN_SUCCESS take effect as early as when transaction "reply" is received and handled in ovsdb_idl_loop_run(). The approach looks good to me, but here is a comment below. > > CC: Han Zhou <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > lib/ovsdb-idl.c | 136 ++++++++++++++++++++++++++++++------------------ > 1 file changed, 85 insertions(+), 51 deletions(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index f54e360e3..400fa3077 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -385,6 +385,7 @@ 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 int 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 +5341,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); This funciton calls poll_immediate_wake(). I think we shouldn't wake here. It is necessary for ovsdb_idl_loop_commit_and_wait() but not here. > + } > + > loop->open_txn = (loop->committing_txn > || ovsdb_idl_get_seqno(loop->idl) == loop->skip_seqno > ? NULL > @@ -5347,6 +5354,83 @@ ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop) > return loop->open_txn; > } > > +/* Attempts to commit the current transaction, if one is open. > + * > + * If a transaction was open, in this or a previous iteration of the main loop, > + * and had not before finished committing (successfully or unsuccessfully), the > + * return value is one of: > + * > + * 1: The transaction committed successfully (or it did not change anything in > + * the database). > + * 0: The transaction failed. > + * -1: The commit is still in progress. > + * > + * Thus, the return value is -1 if the transaction is in progress and otherwise > + * true for success, false for failure. > + * > + * (In the corner case where the IDL sends a transaction to the database and > + * the database commits it, and the connection between the IDL and the database > + * drops before the IDL receives the message confirming the commit, this > + * function can return 0 even though the transaction succeeded.) > + */ > +static int > +ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop) > +{ > + if (!loop->committing_txn) { > + /* Not a meaningful return value: no transaction was in progress. */ > + return 1; > + } > + > + int retval; > + 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; > + 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; > + 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 = -1; > + } > + > + return retval; > +} > + > /* 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 +5461,7 @@ 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; > - 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 = -1; > - } > - } else { > - /* Not a meaningful return value: no transaction was in progress. */ > - retval = 1; > - } > - > + int retval = ovsdb_idl_try_commit_loop_txn(loop); > ovsdb_idl_wait(loop->idl); > > return retval; > -- > 2.26.2 >
On Thu, Jun 4, 2020 at 4:43 AM Han Zhou <hzhou@ovn.org> wrote: > On Wed, Jun 3, 2020 at 9:51 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. > > Thanks Numan for addressing this. I think the patch is to trying to make > the TXN_SUCCESS take effect as early as when transaction "reply" is > received and handled in ovsdb_idl_loop_run(). The approach looks good to > me, but here is a comment below. > > > > > CC: Han Zhou <hzhou@ovn.org> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > lib/ovsdb-idl.c | 136 ++++++++++++++++++++++++++++++------------------ > > 1 file changed, 85 insertions(+), 51 deletions(-) > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index f54e360e3..400fa3077 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -385,6 +385,7 @@ 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 int 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 +5341,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); > > This funciton calls poll_immediate_wake(). I think we shouldn't wake here. > It is necessary for ovsdb_idl_loop_commit_and_wait() but not here. > Thanks for the review. I've addressed this in v2 - https://patchwork.ozlabs.org/project/openvswitch/patch/20200604184715.168340-1-numans@ovn.org/ Thanks Numan > > > + } > > + > > loop->open_txn = (loop->committing_txn > > || ovsdb_idl_get_seqno(loop->idl) == > loop->skip_seqno > > ? NULL > > @@ -5347,6 +5354,83 @@ ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop) > > return loop->open_txn; > > } > > > > +/* Attempts to commit the current transaction, if one is open. > > + * > > + * If a transaction was open, in this or a previous iteration of the > main loop, > > + * and had not before finished committing (successfully or > unsuccessfully), the > > + * return value is one of: > > + * > > + * 1: The transaction committed successfully (or it did not change > anything in > > + * the database). > > + * 0: The transaction failed. > > + * -1: The commit is still in progress. > > + * > > + * Thus, the return value is -1 if the transaction is in progress and > otherwise > > + * true for success, false for failure. > > + * > > + * (In the corner case where the IDL sends a transaction to the database > and > > + * the database commits it, and the connection between the IDL and the > database > > + * drops before the IDL receives the message confirming the commit, this > > + * function can return 0 even though the transaction succeeded.) > > + */ > > +static int > > +ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop) > > +{ > > + if (!loop->committing_txn) { > > + /* Not a meaningful return value: no transaction was in > progress. */ > > + return 1; > > + } > > + > > + int retval; > > + 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; > > + 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; > > + 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 = -1; > > + } > > + > > + return retval; > > +} > > + > > /* 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 +5461,7 @@ 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; > > - 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 = -1; > > - } > > - } else { > > - /* Not a meaningful return value: no transaction was in > progress. */ > > - retval = 1; > > - } > > - > > + int retval = ovsdb_idl_try_commit_loop_txn(loop); > > ovsdb_idl_wait(loop->idl); > > > > return retval; > > -- > > 2.26.2 > > > _______________________________________________ > 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..400fa3077 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -385,6 +385,7 @@ 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 int 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 +5341,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 +5354,83 @@ ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop) return loop->open_txn; } +/* Attempts to commit the current transaction, if one is open. + * + * If a transaction was open, in this or a previous iteration of the main loop, + * and had not before finished committing (successfully or unsuccessfully), the + * return value is one of: + * + * 1: The transaction committed successfully (or it did not change anything in + * the database). + * 0: The transaction failed. + * -1: The commit is still in progress. + * + * Thus, the return value is -1 if the transaction is in progress and otherwise + * true for success, false for failure. + * + * (In the corner case where the IDL sends a transaction to the database and + * the database commits it, and the connection between the IDL and the database + * drops before the IDL receives the message confirming the commit, this + * function can return 0 even though the transaction succeeded.) + */ +static int +ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop) +{ + if (!loop->committing_txn) { + /* Not a meaningful return value: no transaction was in progress. */ + return 1; + } + + int retval; + 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; + 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; + 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 = -1; + } + + return retval; +} + /* 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 +5461,7 @@ 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; - 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 = -1; - } - } else { - /* Not a meaningful return value: no transaction was in progress. */ - retval = 1; - } - + int retval = ovsdb_idl_try_commit_loop_txn(loop); ovsdb_idl_wait(loop->idl); return retval;