From patchwork Wed Jun 3 16:51:39 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1303106 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.137; helo=fraxinus.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 fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 49cZgx4bKWz9sSc for ; Thu, 4 Jun 2020 02:51:57 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id ACDA186C4B; Wed, 3 Jun 2020 16:51:55 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GKA6C4W8QwWE; Wed, 3 Jun 2020 16:51:54 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id A301585161; Wed, 3 Jun 2020 16:51:54 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 82869C0178; Wed, 3 Jun 2020 16:51:54 +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 C83D3C016E for ; Wed, 3 Jun 2020 16:51:52 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id AFF7687D60 for ; Wed, 3 Jun 2020 16:51:52 +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 nxQ4nUGsjpTh for ; Wed, 3 Jun 2020 16:51:51 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by whitealder.osuosl.org (Postfix) with ESMTPS id 1EDCB87D54 for ; Wed, 3 Jun 2020 16:51:50 +0000 (UTC) X-Originating-IP: 115.99.86.122 Received: from nusiddiq.home.org.home.org (unknown [115.99.86.122]) (Authenticated sender: numans@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id E026440005; Wed, 3 Jun 2020 16:51:46 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Wed, 3 Jun 2020 22:21:39 +0530 Message-Id: <20200603165139.1107659-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Cc: Han Zhou Subject: [ovs-dev] [PATCH ovs] 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 | 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); + } + 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;