From patchwork Thu Jul 14 10:37:00 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ales Musil X-Patchwork-Id: 1656337 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LkTDF++C; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=smtp1.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4Lk9sj0QpFz9ryY for ; Thu, 14 Jul 2022 20:37:13 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id DD06F81426; Thu, 14 Jul 2022 10:37:10 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org DD06F81426 Authentication-Results: smtp1.osuosl.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LkTDF++C X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Wrc2Ck_IbS2t; Thu, 14 Jul 2022 10:37:09 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp1.osuosl.org (Postfix) with ESMTPS id BE71080D2B; Thu, 14 Jul 2022 10:37:08 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org BE71080D2B Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7D9E2C0033; Thu, 14 Jul 2022 10:37:08 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 2117FC002D for ; Thu, 14 Jul 2022 10:37:07 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id CF8BB416EB for ; Thu, 14 Jul 2022 10:37:06 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org CF8BB416EB Authentication-Results: smtp2.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LkTDF++C X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id QsXnQhPnBDuR for ; Thu, 14 Jul 2022 10:37:05 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org A27A7416D0 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp2.osuosl.org (Postfix) with ESMTPS id A27A7416D0 for ; Thu, 14 Jul 2022 10:37:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1657795024; 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=nowwxnO1X5xP5n2vjXxCpQ5c0/GQbqx7eCNBUFCMQKk=; b=LkTDF++Cyyjo7/u/04PovlBjtzljuHwG/z6V3ZtPOi1zfkUGjBuJVw9p+c6lF8UyEY8GpN VSkb2AAgfBVSEyMPt4QC15ocXJqgMf5NO+VTgJwe2/WSgON3zlVa8D7DlzJ+IBAVmGzgOU xUFKvwTZzaKdCIj9kP1XxTpAdYyjl1I= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-648-IUZNbZ_VM_aDUp3Daw2ynA-1; Thu, 14 Jul 2022 06:37:03 -0400 X-MC-Unique: IUZNbZ_VM_aDUp3Daw2ynA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 20B751C05AA4 for ; Thu, 14 Jul 2022 10:37:03 +0000 (UTC) Received: from amusil.redhat.com (unknown [10.40.192.14]) by smtp.corp.redhat.com (Postfix) with ESMTP id C0847C15D67; Thu, 14 Jul 2022 10:37:01 +0000 (UTC) From: Ales Musil To: dev@openvswitch.org Date: Thu, 14 Jul 2022 12:37:00 +0200 Message-Id: <20220714103700.1204573-1-amusil@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.85 on 10.11.54.8 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=amusil@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH ovn v5] controller: Add delay after multicast ARP packet 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" The ovn-controller had a race condition over MAC binding table with other controllers. When multiple controllers received GARP from single source usually the one who was able to win the race put it into SB. The others got transaction error which triggered full recompute even if it's not needed. In order to reduce the chance of multiple controllers trying to insert same row at the same time add slight delay to the MAC binding processing. This delay is random interval between 1-50 in ms. This greatly reduces the chance that multiple controllers will try to add MAC binding at exactly the same time. This applies only to multicast ARP request which applies only to GARP that is sent to broadcast address. Local testing with this delay vs without show significantly reduced chance of hitting the transaction error. During the testing 10 GARPs was sent to two controllers at the same time. Without proposed fix at least one controller had multiple transaction errors present every test iteration. With the proposed fix the transaction error was reduced to a single one when it happened which was usually in less than 10% of the iterations. As was mentioned before the race condition can still happen, but the chance is greatly reduced. Suggested-by: Daniel Alvarez Sanchez Signed-off-by: Ales Musil Acked-by: Dumitru Ceara --- v3: Rebase on top of current main. Change the delay to be per ARP request instead of single delay for everything. This allows the possibility to delay only multicast/broadcast AP as suggested by Han. v4,v5: Address comments from Dumitru. --- controller/mac-learn.c | 41 +++++++++++++++++++++++++++++++++-------- controller/mac-learn.h | 9 +++++++-- controller/pinctrl.c | 27 ++++++++++++++++++--------- tests/ovn.at | 6 +++--- 4 files changed, 61 insertions(+), 22 deletions(-) diff --git a/controller/mac-learn.c b/controller/mac-learn.c index 27634dca8..a27607016 100644 --- a/controller/mac-learn.c +++ b/controller/mac-learn.c @@ -18,14 +18,18 @@ #include "mac-learn.h" /* OpenvSwitch lib includes. */ +#include "openvswitch/poll-loop.h" #include "openvswitch/vlog.h" #include "lib/packets.h" +#include "lib/random.h" #include "lib/smap.h" +#include "lib/timeval.h" VLOG_DEFINE_THIS_MODULE(mac_learn); #define MAX_MAC_BINDINGS 1000 #define MAX_FDB_ENTRIES 1000 +#define MAX_MAC_BINDING_DELAY_MSEC 50 static size_t mac_binding_hash(uint32_t dp_key, uint32_t port_key, struct in6_addr *); @@ -46,25 +50,19 @@ ovn_mac_bindings_init(struct hmap *mac_bindings) } void -ovn_mac_bindings_flush(struct hmap *mac_bindings) +ovn_mac_bindings_destroy(struct hmap *mac_bindings) { struct mac_binding *mb; HMAP_FOR_EACH_POP (mb, hmap_node, mac_bindings) { free(mb); } -} - -void -ovn_mac_bindings_destroy(struct hmap *mac_bindings) -{ - ovn_mac_bindings_flush(mac_bindings); hmap_destroy(mac_bindings); } struct mac_binding * ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t dp_key, uint32_t port_key, struct in6_addr *ip, - struct eth_addr mac) + struct eth_addr mac, bool is_unicast) { uint32_t hash = mac_binding_hash(dp_key, port_key, ip); @@ -75,10 +73,13 @@ ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t dp_key, return NULL; } + uint32_t delay = is_unicast + ? 0 : random_range(MAX_MAC_BINDING_DELAY_MSEC) + 1; mb = xmalloc(sizeof *mb); mb->dp_key = dp_key; mb->port_key = port_key; mb->ip = *ip; + mb->commit_at_ms = time_msec() + delay; hmap_insert(mac_bindings, &mb->hmap_node, hash); } mb->mac = mac; @@ -86,6 +87,30 @@ ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t dp_key, return mb; } +/* This is called from ovn-controller main context */ +void +ovn_mac_binding_wait(struct hmap *mac_bindings) +{ + struct mac_binding *mb; + + HMAP_FOR_EACH (mb, hmap_node, mac_bindings) { + poll_timer_wait_until(mb->commit_at_ms); + } +} + +void +ovn_mac_binding_remove(struct mac_binding *mb, struct hmap *mac_bindings) +{ + hmap_remove(mac_bindings, &mb->hmap_node); + free(mb); +} + +bool +ovn_mac_binding_can_commit(const struct mac_binding *mb, long long now) +{ + return now >= mb->commit_at_ms; +} + /* fdb functions. */ void ovn_fdb_init(struct hmap *fdbs) diff --git a/controller/mac-learn.h b/controller/mac-learn.h index e7e8ba2d3..57c50c58b 100644 --- a/controller/mac-learn.h +++ b/controller/mac-learn.h @@ -31,16 +31,21 @@ struct mac_binding { /* Value. */ struct eth_addr mac; + + /* Timestamp when to commit to SB. */ + long long commit_at_ms; }; void ovn_mac_bindings_init(struct hmap *mac_bindings); -void ovn_mac_bindings_flush(struct hmap *mac_bindings); void ovn_mac_bindings_destroy(struct hmap *mac_bindings); +void ovn_mac_binding_wait(struct hmap *mac_bindings); +void ovn_mac_binding_remove(struct mac_binding *mb, struct hmap *mac_bindings); +bool ovn_mac_binding_can_commit(const struct mac_binding *mb, long long now); struct mac_binding *ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t dp_key, uint32_t port_key, struct in6_addr *ip, - struct eth_addr mac); + struct eth_addr mac, bool is_unicast); diff --git a/controller/pinctrl.c b/controller/pinctrl.c index f954362b7..beb3d3344 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -4134,9 +4134,13 @@ pinctrl_handle_put_mac_binding(const struct flow *md, memcpy(&ip_key, &ip6, sizeof ip_key); } + /* If the ARP reply was unicast we should not delay it, + * there won't be any race. */ + bool is_unicast = !eth_addr_is_multicast(headers->dl_dst); struct mac_binding *mb = ovn_mac_binding_add(&put_mac_bindings, dp_key, port_key, &ip_key, - headers->dl_src); + headers->dl_src, + is_unicast); if (!mb) { COVERAGE_INC(pinctrl_drop_put_mac_binding); return; @@ -4296,14 +4300,18 @@ run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn, return; } - const struct mac_binding *mb; - HMAP_FOR_EACH (mb, hmap_node, &put_mac_bindings) { - run_put_mac_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key, - sbrec_port_binding_by_key, - sbrec_mac_binding_by_lport_ip, - mb); + long long now = time_msec(); + + struct mac_binding *mb; + HMAP_FOR_EACH_SAFE (mb, hmap_node, &put_mac_bindings) { + if (ovn_mac_binding_can_commit(mb, now)) { + run_put_mac_binding(ovnsb_idl_txn, + sbrec_datapath_binding_by_key, + sbrec_port_binding_by_key, + sbrec_mac_binding_by_lport_ip, mb); + ovn_mac_binding_remove(mb, &put_mac_bindings); + } } - ovn_mac_bindings_flush(&put_mac_bindings); } static void @@ -4352,9 +4360,10 @@ run_buffered_binding(struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn) + OVS_REQUIRES(pinctrl_mutex) { if (ovnsb_idl_txn && !hmap_is_empty(&put_mac_bindings)) { - poll_immediate_wake(); + ovn_mac_binding_wait(&put_mac_bindings); } } diff --git a/tests/ovn.at b/tests/ovn.at index c346975e6..eff18fb84 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -23002,7 +23002,7 @@ send_garp 1 1 $eth_src $eth_dst $spa $tpa wait_row_count MAC_Binding 1 -AT_CHECK([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ +OVS_WAIT_UNTIL([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ list mac_binding], [0], [lr0-sw0 10.0.0.30 50:54:00:00:00:03 @@ -23049,7 +23049,7 @@ grep table_id=10 | wc -l`]) check_row_count MAC_Binding 1 -AT_CHECK([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ +OVS_WAIT_UNTIL([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ list mac_binding], [0], [lr0-sw0 10.0.0.30 50:54:00:00:00:13 @@ -23078,7 +23078,7 @@ OVS_WAIT_UNTIL( | wc -l`] ) -AT_CHECK([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ +OVS_WAIT_UNTIL([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ find mac_binding ip=10.0.0.50], [0], [lr0-sw0 10.0.0.50 50:54:00:00:00:33