From patchwork Tue Sep 13 19:08:51 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1677461 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.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 ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4MRtLZ0s0Qz1ypR for ; Wed, 14 Sep 2022 05:09:25 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 930EF60B10; Tue, 13 Sep 2022 19:09:22 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 930EF60B10 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 W647AANJa3hW; Tue, 13 Sep 2022 19:09:21 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp3.osuosl.org (Postfix) with ESMTPS id 8584C60B0C; Tue, 13 Sep 2022 19:09:20 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 8584C60B0C Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 34090C007E; Tue, 13 Sep 2022 19:09:20 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 33522C0032 for ; Tue, 13 Sep 2022 19:09:19 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 1AADF408CC for ; Tue, 13 Sep 2022 19:09:19 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 1AADF408CC X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Fwqv75H3GMHm for ; Tue, 13 Sep 2022 19:09:17 +0000 (UTC) X-Greylist: from auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 7316640350 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [IPv6:2001:4b98:dc4:8::231]) by smtp4.osuosl.org (Postfix) with ESMTPS id 7316640350 for ; Tue, 13 Sep 2022 19:09:12 +0000 (UTC) Received: (Authenticated sender: i.maximets@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id 9FEEE100006; Tue, 13 Sep 2022 19:09:05 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Tue, 13 Sep 2022 21:08:51 +0200 Message-Id: <20220913190852.383513-2-i.maximets@ovn.org> X-Mailer: git-send-email 2.37.3 In-Reply-To: <20220913190852.383513-1-i.maximets@ovn.org> References: <20220913190852.383513-1-i.maximets@ovn.org> MIME-Version: 1.0 Cc: Daniel Ding , Ilya Maximets Subject: [ovs-dev] [PATCH v2 1/2] ofproto-dpif-upcall: Add debug commands to pause/resume revalidators. 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" New commands 'revalidator/pause' and 'revalidator/resume'. Not documented, since these should not be used in production environments. Will be used for unit tests in the next commit. Signed-off-by: Ilya Maximets Acked-by: Mike Pattrick --- ofproto/ofproto-dpif-upcall.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 57f94df54..fe4709058 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -362,6 +362,10 @@ static void upcall_unixctl_dump_wait(struct unixctl_conn *conn, int argc, const char *argv[], void *aux); static void upcall_unixctl_purge(struct unixctl_conn *conn, int argc, const char *argv[], void *aux); +static void upcall_unixctl_pause(struct unixctl_conn *conn, int argc, + const char *argv[], void *aux); +static void upcall_unixctl_resume(struct unixctl_conn *conn, int argc, + const char *argv[], void *aux); static struct udpif_key *ukey_create_from_upcall(struct upcall *, struct flow_wildcards *); @@ -434,6 +438,10 @@ udpif_init(void) upcall_unixctl_dump_wait, NULL); unixctl_command_register("revalidator/purge", "", 0, 0, upcall_unixctl_purge, NULL); + unixctl_command_register("revalidator/pause", NULL, 0, 0, + upcall_unixctl_pause, NULL); + unixctl_command_register("revalidator/resume", NULL, 0, 0, + upcall_unixctl_resume, NULL); ovsthread_once_done(&once); } } @@ -3099,6 +3107,31 @@ upcall_unixctl_purge(struct unixctl_conn *conn, int argc OVS_UNUSED, unixctl_command_reply(conn, ""); } +static void +upcall_unixctl_pause(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) +{ + struct udpif *udpif; + + LIST_FOR_EACH (udpif, list_node, &all_udpifs) { + udpif_pause_revalidators(udpif); + } + unixctl_command_reply(conn, ""); +} + +static void +upcall_unixctl_resume(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) +{ + struct udpif *udpif; + + LIST_FOR_EACH (udpif, list_node, &all_udpifs) { + udpif_resume_revalidators(udpif); + } + unixctl_command_reply(conn, ""); +} + + /* Flows are sorted in the following order: * netdev, flow state (offloaded/kernel path), flow_pps_rate. */ From patchwork Tue Sep 13 19:08:52 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1677462 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4MRtLZ2PcTz1ypT for ; Wed, 14 Sep 2022 05:09:24 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 98C1F403EA; Tue, 13 Sep 2022 19:09:19 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 98C1F403EA 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 xfOymCgAl7Pj; Tue, 13 Sep 2022 19:09:18 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp2.osuosl.org (Postfix) with ESMTPS id 77DC840220; Tue, 13 Sep 2022 19:09:17 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 77DC840220 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 47C4FC0032; Tue, 13 Sep 2022 19:09:17 +0000 (UTC) X-Original-To: ovs-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 10905C002D for ; Tue, 13 Sep 2022 19:09:15 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id C9F0940328 for ; Tue, 13 Sep 2022 19:09:14 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org C9F0940328 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 XszJo-TSZXN2 for ; Tue, 13 Sep 2022 19:09:13 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 57ACA40179 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by smtp2.osuosl.org (Postfix) with ESMTPS id 57ACA40179 for ; Tue, 13 Sep 2022 19:09:12 +0000 (UTC) Received: (Authenticated sender: i.maximets@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id B20DA100008; Tue, 13 Sep 2022 19:09:09 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Tue, 13 Sep 2022 21:08:52 +0200 Message-Id: <20220913190852.383513-3-i.maximets@ovn.org> X-Mailer: git-send-email 2.37.3 In-Reply-To: <20220913190852.383513-1-i.maximets@ovn.org> References: <20220913190852.383513-1-i.maximets@ovn.org> MIME-Version: 1.0 Cc: Daniel Ding , Ilya Maximets Subject: [ovs-dev] [PATCH v2 2/2] bond: Avoid deadlock while updating post recirculation rules. 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" If the PACKET_OUT from controller ends up with sending packet to a bond interface, the main thread will take locks in the following order: handle_openflow --> take ofproto_mutex handle_packet_out packet_xlate output_normal bond_update_post_recirc_rules --> take rwlock in bond.c If at the same time revalidator thread is processing other packet with the output to the same bond: xlate_actions output_normal bond_update_post_recirc_rules --> take rwlock in bond.c update_recirc_rules ofproto_dpif_add_internal_flow ofproto_flow_mod --> take ofproto_mutex So, it is possible for these 2 threads to lock each other by taking one lock and waiting for another thread to release the second lock. It is also possible for the main thread to lock itself up by trying to acquire ofproto_mutex for the second time, if it will actually proceed with update_recirc_rules() after taking the bond rwlock. The problem appears to be that bond_update_post_recirc_rules() is called during the flow translation even if side effects are prohibited, which is the case for openflow PACKET_OUT handling. Skipping actual flow updates during the flow translation if side effects are disabled to avoid the deadlock. Since flows are not installed now when actions translated for very first packet, installing initial flows in bond_reconfigure(). This will cover the case of allocating a new recirc_id. Also checking if we need to update flows in bond_run() to cover link state changes. Regression test is added to catch the double lock case. Reported-at: https://github.com/openvswitch/ovs-issues/issues/259 Reported-by: Daniel Ding Fixes: adcf00ba35a0 ("ofproto/bond: Implement bond megaflow using recirculation") Signed-off-by: Ilya Maximets Acked-by: Mike Pattrick --- ofproto/bond.c | 30 ++++++++++++++++ ofproto/bond.h | 3 ++ ofproto/ofproto-dpif-xlate.c | 15 ++++++-- tests/ofproto-dpif.at | 66 ++++++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 3 deletions(-) diff --git a/ofproto/bond.c b/ofproto/bond.c index f06cf20c9..a3cc795bf 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -187,10 +187,15 @@ static struct bond_member *choose_output_member(const struct bond *, uint16_t vlan) OVS_REQ_RDLOCK(rwlock); static void update_recirc_rules__(struct bond *); +static bool bond_may_recirc(const struct bond *bond); +static void bond_update_post_recirc_rules__(struct bond *bond, + const bool force) + OVS_REQ_WRLOCK(rwlock); static bool bond_is_falling_back_to_ab(const struct bond *); static void bond_add_lb_output_buckets(const struct bond *); static void bond_del_lb_output_buckets(const struct bond *); + /* Attempts to parse 's' as the name of a bond balancing mode. If successful, * stores the mode in '*balance' and returns true. Otherwise returns false * without modifying '*balance'. */ @@ -515,6 +520,12 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s) bond_entry_reset(bond); } + if (bond->ofproto->backer->rt_support.odp.recirc + && bond_may_recirc(bond)) { + /* Update rules to reflect possible recirc_id changes. */ + update_recirc_rules(bond); + } + ovs_rwlock_unlock(&rwlock); return revalidate; } @@ -728,6 +739,12 @@ bond_run(struct bond *bond, enum lacp_status lacp_status) bond_choose_active_member(bond); } + if (bond->ofproto->backer->rt_support.odp.recirc + && bond_may_recirc(bond)) { + /* Update rules to reflect possible link state changes. */ + bond_update_post_recirc_rules__(bond, false); + } + revalidate = bond->bond_revalidate; bond->bond_revalidate = false; ovs_rwlock_unlock(&rwlock); @@ -1091,6 +1108,19 @@ bond_update_post_recirc_rules(struct bond *bond, uint32_t *recirc_id, } } +void +bond_get_recirc_id_and_hash_basis(struct bond *bond, uint32_t *recirc_id, + uint32_t *hash_basis) +{ + ovs_rwlock_rdlock(&rwlock); + if (bond_may_recirc(bond)) { + *recirc_id = bond->recirc_id; + *hash_basis = bond->basis; + } else { + *recirc_id = *hash_basis = 0; + } + ovs_rwlock_unlock(&rwlock); +} /* Rebalancing. */ diff --git a/ofproto/bond.h b/ofproto/bond.h index 2eb0c95a7..9c2b994c6 100644 --- a/ofproto/bond.h +++ b/ofproto/bond.h @@ -131,6 +131,9 @@ void bond_rebalance(struct bond *); void bond_update_post_recirc_rules(struct bond *, uint32_t *recirc_id, uint32_t *hash_basis); +void bond_get_recirc_id_and_hash_basis(struct bond *bond, uint32_t *recirc_id, + uint32_t *hash_basis); + bool bond_use_lb_output_action(const struct bond *bond); #endif /* bond.h */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index b695baba9..6d6c9bf25 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2466,9 +2466,18 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle, /* In case recirculation is not actually in use, 'xr.recirc_id' * will be set to '0', since a valid 'recirc_id' can * not be zero. */ - bond_update_post_recirc_rules(out_xbundle->bond, - &xr.recirc_id, - &xr.hash_basis); + if (ctx->xin->allow_side_effects) { + bond_update_post_recirc_rules(out_xbundle->bond, + &xr.recirc_id, + &xr.hash_basis); + } else { + /* If side effects are not allowed, only getting the bond + * configuration. Rule updates will be handled by the + * main thread later. */ + bond_get_recirc_id_and_hash_basis(out_xbundle->bond, + &xr.recirc_id, + &xr.hash_basis); + } if (xr.recirc_id) { /* Use recirculation instead of output. */ use_recirc = true; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 02533f11d..b2e221b89 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -531,6 +531,72 @@ AT_CHECK([sed -n '/member p2/,/^$/p' bond3.txt | grep 'hash'], [0], [ignore]) OVS_VSWITCHD_STOP() AT_CLEANUP +dnl Regression test for a deadlock / double lock on post-recirculation rule +dnl updates while processing PACKET_OUT. +AT_SETUP([ofproto-dpif - balance-tcp bonding rule updates on packet-out]) +dnl Create br0 with interfaces bond0(p1, p2) and p5, +dnl and br1 with interfaces bond1(p3, p4) and p6. +dnl bond0 <-> bond1 +OVS_VSWITCHD_START( + [add-bond br0 bond0 p1 p2 bond_mode=balance-tcp lacp=active dnl + other-config:lacp-time=fast other-config:bond-rebalance-interval=1000 -- dnl + set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p1.sock ofport_request=1 mtu_request=65535 -- dnl + set interface p2 type=dummy options:pstream=punix:$OVS_RUNDIR/p2.sock ofport_request=2 mtu_request=65535 -- dnl + add-port br0 p5 -- set interface p5 ofport_request=5 type=dummy mtu_request=65535 -- dnl + add-br br1 -- dnl + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- dnl + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 dnl + fail-mode=secure -- dnl + add-bond br1 bond1 p3 p4 bond_mode=balance-tcp lacp=active dnl + other-config:lacp-time=fast other-config:bond-rebalance-interval=1000 -- dnl + set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock ofport_request=3 mtu_request=65535 -- dnl + set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock ofport_request=4 mtu_request=65535 -- dnl + add-port br1 p6 -- set interface p6 ofport_request=6 type=dummy mtu_request=65535 --]) +AT_CHECK([ovs-appctl vlog/set bond:dbg]) +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK +]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl add-flow br1 action=normal]) +OVS_WAIT_WHILE([ovs-appctl bond/show | grep "may_enable: false"]) + +ovs-appctl time/stop +ovs-appctl time/warp 2000 200 + +dnl Send some traffic to distribute all the hashes between ports. +AT_CHECK([SEND_TCP_BOND_PKTS([p5], [5], [65500])]) + +dnl Wait for rebalancing for per-hash stats accounting. +ovs-appctl time/warp 1000 100 + +dnl Check that p2 handles some hashes. +ovs-appctl bond/show > bond1.txt +AT_CHECK([sed -n '/member p2/,/^$/p' bond1.txt | grep 'hash'], [0], [ignore]) + +dnl Pause revalidators to be sure that they do not update flows while +dnl the bonding configuration chnages. +ovs-appctl revalidator/pause + +dnl Move p2 down to trigger update of bonding post-recirculation rules by +dnl forcing move of all the hashes to p1. +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state p2 down], 0, [OK +]) + +dnl Send PACKET_OUT that may lead to flow updates since the bonding +dnl configuration changed. +packet=ffffffffffff00102030405008004500001c00000000401100000a000002ffffffff0035111100080000 +AT_CHECK([ovs-ofctl packet-out br0 "in_port=p5 packet=$packet actions=resubmit(,0)"]) + +dnl Resume revalidators. +ovs-appctl revalidator/resume +ovs-appctl revalidator/wait + +ovs-appctl time/warp 200 100 +dnl Check that all hashes moved form p2 and OVS is still working. +ovs-appctl bond/show > bond2.txt +AT_CHECK([sed -n '/member p2/,/^$/p' bond2.txt | grep 'hash'], [1], [ignore]) + +OVS_VSWITCHD_STOP() +AT_CLEANUP # Makes sure recirculation does not change the way packet is handled. AT_SETUP([ofproto-dpif - balance-tcp bonding, different recirc flow ])