From patchwork Thu Jun 4 18:47:15 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1303693 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 49dFBw3kSmz9sSc for ; Fri, 5 Jun 2020 04:47:36 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id B1D8C883CC; Thu, 4 Jun 2020 18:47:33 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xFkXtyYRxxRE; Thu, 4 Jun 2020 18:47:32 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 08433883C4; Thu, 4 Jun 2020 18:47:32 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id DF73CC0888; Thu, 4 Jun 2020 18:47:31 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4EBB6C016E for ; Thu, 4 Jun 2020 18:47:30 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 3CE66883CC for ; Thu, 4 Jun 2020 18:47:30 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oatwEwUvVIMM for ; Thu, 4 Jun 2020 18:47:29 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by whitealder.osuosl.org (Postfix) with ESMTPS id 91A09883C4 for ; Thu, 4 Jun 2020 18:47:28 +0000 (UTC) X-Originating-IP: 115.99.186.120 Received: from nusiddiq.home.org.com (unknown [115.99.186.120]) (Authenticated sender: numans@ovn.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 290F31BF204; Thu, 4 Jun 2020 18:47:23 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Fri, 5 Jun 2020 00:17:15 +0530 Message-Id: <20200604184715.168340-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Cc: Han Zhou Subject: [ovs-dev] [PATCH ovs v2] ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" From: Numan Siddique 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 Signed-off-by: Numan Siddique --- 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;