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?
Thanks,
Han
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
