From patchwork Mon Nov 18 11:46:20 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1196717 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.133; helo=hemlock.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="SVedm6f/"; dkim-atps=neutral Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47GnH2566cz9sPW for ; Mon, 18 Nov 2019 22:46:38 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 159EE87D35; Mon, 18 Nov 2019 11:46:36 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9uuIiyC6P3vn; Mon, 18 Nov 2019 11:46:33 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 6CF5C87CFB; Mon, 18 Nov 2019 11:46:33 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4099DC18DA; Mon, 18 Nov 2019 11:46:33 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 73055C07AC for ; Mon, 18 Nov 2019 11:46:31 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 5D02D857F8 for ; Mon, 18 Nov 2019 11:46:31 +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 EDL4T7ntWiDm for ; Mon, 18 Nov 2019 11:46:30 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 5ABB5857A3 for ; Mon, 18 Nov 2019 11:46:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1574077589; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=u8iCwiQBi+REsdTejRIa8W3FzuSQ6dXOt0n1thyemSM=; b=SVedm6f/+m91FZMe2yz5cfY6afVPxb03YQLpfEyt1i2zTKj0F8E287u89jIP1y29M290Ml VaRjpNbpRqGGYj2oUgKC+RLdoNOD/yJMMGJ1QY8+tXCT+gETxBNmauVFITilo/TSqfh6un byuxcNjUbWwM09iO8NdogNX1bRCS0vI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-228-CMjDtQ4VPxi_RBIz1q9oCQ-1; Mon, 18 Nov 2019 06:46:26 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2E151189C774; Mon, 18 Nov 2019 11:46:25 +0000 (UTC) Received: from dceara.remote.csb (ovpn-116-115.ams2.redhat.com [10.36.116.115]) by smtp.corp.redhat.com (Postfix) with ESMTP id B4D4310027B3; Mon, 18 Nov 2019 11:46:23 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Mon, 18 Nov 2019 12:46:20 +0100 Message-Id: <20191118114615.31047.92445.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-MC-Unique: CMjDtQ4VPxi_RBIz1q9oCQ-1 X-Mimecast-Spam-Score: 0 Cc: hzhou@ovn.org Subject: [ovs-dev] [PATCH v4 ovn 1/4] ovn-controller: Refactor I-P engine_run() tracking. 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" This commit simplifies the logic of calling engine_run and engine_need_run in order to reduce the number of external variables required to track the result of the last engine execution. The engine code is also refactored a bit and the engine_run() function is split in different functions that handle computing/recomputing a node. Signed-off-by: Dumitru Ceara --- controller/ovn-controller.c | 33 ++++++----- lib/inc-proc-eng.c | 124 +++++++++++++++++++++++++++++-------------- lib/inc-proc-eng.h | 7 ++ 3 files changed, 107 insertions(+), 57 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 9ab98be..3922f3d 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1942,7 +1942,6 @@ main(int argc, char *argv[]) &pending_pkt); uint64_t engine_run_id = 0; - uint64_t old_engine_run_id = 0; bool engine_run_done = true; unsigned int ovs_cond_seqno = UINT_MAX; @@ -1952,10 +1951,11 @@ main(int argc, char *argv[]) exiting = false; restart = false; while (!exiting) { + engine_run_id++; + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); - old_engine_run_id = engine_run_id; struct ovsdb_idl_txn *ovs_idl_txn = ovsdb_idl_loop_run(&ovs_idl_loop); unsigned int new_ovs_cond_seqno @@ -2047,12 +2047,12 @@ main(int argc, char *argv[]) if (engine_run_done) { engine_set_abort_recompute(true); engine_run_done = engine_run(&en_flow_output, - ++engine_run_id); + engine_run_id); } } else { engine_set_abort_recompute(false); engine_run_done = true; - engine_run(&en_flow_output, ++engine_run_id); + engine_run(&en_flow_output, engine_run_id); } } stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, @@ -2095,17 +2095,20 @@ main(int argc, char *argv[]) } } - if (old_engine_run_id == engine_run_id || !engine_run_done) { - if (!engine_run_done || engine_need_run(&en_flow_output)) { - VLOG_DBG("engine did not run, force recompute next time: " - "br_int %p, chassis %p", br_int, chassis); - engine_set_force_recompute(true); - poll_immediate_wake(); - } else { - VLOG_DBG("engine did not run, and it was not needed" - " either: br_int %p, chassis %p", - br_int, chassis); - } + if (engine_need_run(&en_flow_output, engine_run_id)) { + VLOG_DBG("engine did not run, force recompute next time: " + "br_int %p, chassis %p", br_int, chassis); + engine_set_force_recompute(true); + poll_immediate_wake(); + } else if (!engine_run_done) { + VLOG_DBG("engine was aborted, force recompute next time: " + "br_int %p, chassis %p", br_int, chassis); + engine_set_force_recompute(true); + poll_immediate_wake(); + } else if (!engine_has_run(&en_flow_output, engine_run_id)) { + VLOG_DBG("engine did not run, and it was not needed" + " either: br_int %p, chassis %p", + br_int, chassis); } else { engine_set_force_recompute(false); } diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c index 1064a08..8a085e2 100644 --- a/lib/inc-proc-eng.c +++ b/lib/inc-proc-eng.c @@ -129,14 +129,72 @@ engine_ovsdb_node_add_index(struct engine_node *node, const char *name, } bool -engine_run(struct engine_node *node, uint64_t run_id) +engine_has_run(struct engine_node *node, uint64_t run_id) +{ + return node->run_id == run_id; +} + +/* Do a full recompute (or at least try). If we're not allowed then + * mark the node as "aborted". + */ +static bool +engine_recompute(struct engine_node *node, bool forced, bool allowed) +{ + VLOG_DBG("node: %s, recompute (%s)", node->name, + forced ? "forced" : "triggered"); + + if (!allowed) { + VLOG_DBG("node: %s, recompute aborted", node->name); + return false; + } + + node->run(node); + VLOG_DBG("node: %s, changed: %d", node->name, node->changed); + return true; +} + +/* Return true if the node could be computed without triggerring a full + * recompute. + */ +static bool +engine_compute(struct engine_node *node, bool recompute_allowed) +{ + for (size_t i = 0; i < node->n_inputs; i++) { + /* If the input node data changed call its change handler. */ + if (node->inputs[i].node->changed) { + VLOG_DBG("node: %s, handle change for input %s", + node->name, node->inputs[i].node->name); + + /* If the input change can't be handled incrementally, run + * the node handler. + */ + if (!node->inputs[i].change_handler(node)) { + VLOG_DBG("node: %s, can't handle change for input %s, " + "fall back to recompute", + node->name, node->inputs[i].node->name); + if (!engine_recompute(node, false, recompute_allowed)) { + return false; + } + } + } + } + + return true; +} + +bool engine_run(struct engine_node *node, uint64_t run_id) { if (node->run_id == run_id) { + /* The node was already updated in this run (could be input for + * multiple other nodes). Stop processing. + */ return true; } - node->run_id = run_id; + /* Initialize the node for this run. */ + node->run_id = run_id; node->changed = false; + if (!node->n_inputs) { node->run(node); VLOG_DBG("node: %s, changed: %d", node->name, node->changed); @@ -150,59 +208,45 @@ engine_run(struct engine_node *node, uint64_t run_id) } bool need_compute = false; - bool need_recompute = false; if (engine_force_recompute) { - need_recompute = true; - } else { - for (size_t i = 0; i < node->n_inputs; i++) { - if (node->inputs[i].node->changed) { - need_compute = true; - if (!node->inputs[i].change_handler) { - need_recompute = true; - break; - } - } - } + return engine_recompute(node, true, !engine_abort_recompute); } - if (need_recompute) { - VLOG_DBG("node: %s, recompute (%s)", node->name, - engine_force_recompute ? "forced" : "triggered"); - if (engine_abort_recompute) { - VLOG_DBG("node: %s, recompute aborted", node->name); - return false; - } - node->run(node); - } else if (need_compute) { - for (size_t i = 0; i < node->n_inputs; i++) { - if (node->inputs[i].node->changed) { - VLOG_DBG("node: %s, handle change for input %s", - node->name, node->inputs[i].node->name); - if (!node->inputs[i].change_handler(node)) { - VLOG_DBG("node: %s, can't handle change for input %s, " - "fall back to recompute", - node->name, node->inputs[i].node->name); - if (engine_abort_recompute) { - VLOG_DBG("node: %s, recompute aborted", node->name); - return false; - } - node->run(node); - break; - } + /* If any of the inputs updated data but there is no change_handler, then + * recompute the current node too. + */ + for (size_t i = 0; i < node->n_inputs; i++) { + if (node->inputs[i].node->changed) { + need_compute = true; + + /* Trigger a recompute if we don't have a change handler. */ + if (!node->inputs[i].change_handler) { + return engine_recompute(node, false, !engine_abort_recompute); } } } + if (need_compute) { + /* If we couldn't compute the node we either aborted or triggered + * a full recompute. In any case, stop processing. + */ + return engine_compute(node, !engine_abort_recompute); + } + VLOG_DBG("node: %s, changed: %d", node->name, node->changed); return true; } bool -engine_need_run(struct engine_node *node) +engine_need_run(struct engine_node *node, uint64_t run_id) { size_t i; + if (node->run_id == run_id) { + return false; + } + if (!node->n_inputs) { node->run(node); VLOG_DBG("input node: %s, changed: %d", node->name, node->changed); @@ -210,7 +254,7 @@ engine_need_run(struct engine_node *node) } for (i = 0; i < node->n_inputs; i++) { - if (engine_need_run(node->inputs[i].node)) { + if (engine_need_run(node->inputs[i].node, run_id)) { return true; } } diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 3a69dc2..abd41b2 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -130,9 +130,9 @@ bool engine_run(struct engine_node *, uint64_t run_id); * terminates. */ void engine_cleanup(struct engine_node *); -/* Check if engine needs to run, i.e. any change to be processed. */ +/* Check if engine needs to run but didn't. */ bool -engine_need_run(struct engine_node *); +engine_need_run(struct engine_node *, uint64_t run_id); /* Get the input node with for */ struct engine_node * engine_get_input(const char *input_name, @@ -159,6 +159,9 @@ const struct engine_context * engine_get_context(void); void engine_set_context(const struct engine_context *); +/* Return true if the engine has run for 'node' in the 'run_id' iteration. */ +bool engine_has_run(struct engine_node *node, uint64_t run_id); + struct ed_ovsdb_index { const char *name; struct ovsdb_idl_index *index;