[ovs-dev] [PATCH ovs v2] ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run.

Han Zhou hzhou at ovn.org
Thu Jun 4 19:08:56 UTC 2020


On Thu, Jun 4, 2020 at 11:47 AM <numans at ovn.org> wrote:
>
> From: Numan Siddique <numans at 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 at ovn.org>
> Signed-off-by: Numan Siddique <numans at 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


More information about the dev mailing list