[ovs-dev] [PATCH ovs v2] ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run.
Numan Siddique
numans at ovn.org
Fri Jun 5 08:31:26 UTC 2020
On Fri, Jun 5, 2020 at 12:39 AM Han Zhou <hzhou at ovn.org> wrote:
> 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?
>
This makes more sense. I submitted v3.
Thanks
Numan
>
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list