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

Numan Siddique numans at ovn.org
Thu Jun 4 18:48:49 UTC 2020


On Thu, Jun 4, 2020 at 4:43 AM Han Zhou <hzhou at ovn.org> wrote:

> On Wed, Jun 3, 2020 at 9:51 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.
>
> 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 at ovn.org>
> > Signed-off-by: Numan Siddique <numans at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list