From patchwork Fri Oct 2 19:47:52 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1441562 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.136; helo=smtp3.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Dh7KT46pJz9sRf for ; Thu, 18 Feb 2021 19:34:05 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id D7AE56062E for ; Thu, 18 Feb 2021 08:34:03 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id iFF0iIEix3-N for ; Thu, 18 Feb 2021 08:34:01 +0000 (UTC) Received: by smtp3.osuosl.org (Postfix, from userid 1001) id BACBA605F5; Thu, 18 Feb 2021 08:34:01 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp3.osuosl.org (Postfix) with ESMTP id 2F2C8605FF; Thu, 18 Feb 2021 08:33:35 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id D8F48C0010; Thu, 18 Feb 2021 08:33:34 +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 9C45CC000E for ; Thu, 18 Feb 2021 08:33:33 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 9675A86B78 for ; Thu, 18 Feb 2021 08:33: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 DcmlK9VPZ5de for ; Thu, 18 Feb 2021 08:33:33 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from ti0189a330-0925.bb.online.no (ti0189a330-0925.bb.online.no [88.88.218.161]) by whitealder.osuosl.org (Postfix) with ESMTP id 3C74F86A10 for ; Thu, 18 Feb 2021 08:33:10 +0000 (UTC) X-Mailbox-Line: From 71c7d2dcde55ad14532d87345a7a6d7c1814b614 Mon Sep 17 00:00:00 2001 Message-Id: <71c7d2dcde55ad14532d87345a7a6d7c1814b614.1613635760.git.frode.nordahl@canonical.com> In-Reply-To: References: From: Han Zhou To: dev@openvswitch.org Date: Fri, 2 Oct 2020 12:47:52 -0700 Subject: [ovs-dev] [PATCH ovn branch-20.06 07/15] ofctrl.c: Avoid repeatedly linking an installed flow and a desired flow. 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" In update_installed_flows_by_compare() there are two loops. The first loop iterates the installed flows and find its peer in desired flows to: 1. uninstall flows that are not needed anymore 2. update flows if needed At the same time, it links the desired flow found for the installed flow which also set the desired flow as the current active installed flow. The second loop iterates the desired flows and find its peer in installed flows to install missing flows. At the same time it will detect if there are conflict desired flows matching same installed flow then just link them. However, currently in the second loop, it blindly link the desired flows to the installed flows, without checking if it is already linked in the first loop. Lucky enough, this won't cause any real problem so far, because when there are conflict flows, the one found in the first loop will be set as active in the installed_flow, and in the function link_installed_to_desired() checks if it is already the active desired flow it just does nothing but return. However, the check in the link_installed_to_desired() is confusing because a desired_flow may be linked to the installed_flow already but not the active flow, and the check is insufficient. It should be rather an assertion and let the caller ensure that a pair of desired_flow and installed_flow is never linked twice. For the above reason, this patch does the following changes: 1. Removes the check in link_installed_to_desired() and convert it to an assert. 2. Before calling link_installed_to_desired() in the above mentioned loop, check if the desired flow is already installed. Acked-by: Dumitru Ceara Signed-off-by: Han Zhou (cherry picked from commit 7cab7bd1268ba67429954da4f73de91090acf779) Signed-off-by: Frode Nordahl --- controller/ofctrl.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 3225e6c67..384659d0c 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -806,13 +806,18 @@ desired_flow_set_active(struct desired_flow *d) d->installed_flow->desired_flow = d; } +/* Adds the desired flow to the list of desired flows that have same match + * conditions as the installed flow. + * + * If the newly added desired flow is the first one in the list, it is also set + * as the active one. + * + * It is caller's responsibility to make sure the link between the pair didn't + * exist before. */ static void link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) { - if (i->desired_flow == d) { - return; - } - + ovs_assert(i->desired_flow != d); if (ovs_list_is_empty(&i->desired_refs)) { ovs_assert(!i->desired_flow); i->desired_flow = d; @@ -1706,8 +1711,12 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, /* Copy 'd' from 'flow_table' to installed_flows. */ i = installed_flow_dup(d); hmap_insert(&installed_flows, &i->match_hmap_node, i->flow.hash); + link_installed_to_desired(i, d); + } else if (!d->installed_flow) { + /* This is a desired_flow that conflicts with one installed + * previously but not linked yet. */ + link_installed_to_desired(i, d); } - link_installed_to_desired(i, d); } }