From patchwork Fri Jun 9 15:03:31 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peng He X-Patchwork-Id: 1793270 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.133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20221208 header.b=KGDeJl1u; dkim-atps=neutral Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.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 4QdMYB0Vv9z20Wg for ; Sat, 10 Jun 2023 12:37:18 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id B5C4341A59; Sat, 10 Jun 2023 02:37:15 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org B5C4341A59 Authentication-Results: smtp2.osuosl.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20221208 header.b=KGDeJl1u 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 r3EaOnEeZzf9; Sat, 10 Jun 2023 02:37:14 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp2.osuosl.org (Postfix) with ESMTPS id A9E3E400DD; Sat, 10 Jun 2023 02:37:13 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org A9E3E400DD Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7FD9EC007A; Sat, 10 Jun 2023 02:37:13 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) by lists.linuxfoundation.org (Postfix) with ESMTP id CD61BC0029 for ; Sat, 10 Jun 2023 02:37:12 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id A78D384322 for ; Sat, 10 Jun 2023 02:37:12 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org A78D384322 Authentication-Results: smtp1.osuosl.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20221208 header.b=KGDeJl1u 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 Ic8-5J9a2WVg for ; Sat, 10 Jun 2023 02:37:12 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org E276B8431F Received: from mail-pl1-x629.google.com (mail-pl1-x629.google.com [IPv6:2607:f8b0:4864:20::629]) by smtp1.osuosl.org (Postfix) with ESMTPS id E276B8431F for ; Sat, 10 Jun 2023 02:37:11 +0000 (UTC) Received: by mail-pl1-x629.google.com with SMTP id d9443c01a7336-1b2439e9004so11416885ad.3 for ; Fri, 09 Jun 2023 19:37:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686364631; x=1688956631; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=57ZW5d1WLcxQ6osMSoV5Q2YYM+lVRgkZTG+SbiGXI1E=; b=KGDeJl1uNxGjJqL8Uz8lvWwIkNyd9Rhp7bFnmsdtGLkcazsS8OgWhK++NJdk0feWqs c32qBwrpiE5CI4B7P2fZ6qtD5dVJS13mHXGHq5UOuEKjEGXNAYKdsuRRjCmZ/9GTQl+q 8MkrdhiLINny0BG9DPtXOJ+WhqiARtBqXUdIQw8LiaccP9zo2I0y1RW80ZKwSUDBptaO GSCebDhemEOPD3liId0fPgDZQYzai+8vb20xZgywUE+s0M95uc0pTZxExoXiXZLYNBqo KNQWmIfr7JKLfUVglwrE+QQcN/HL0x+MWFMGZQFmQol9A7Bk7n7PiQJYkPUkfdKijqBu Kkpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686364631; x=1688956631; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=57ZW5d1WLcxQ6osMSoV5Q2YYM+lVRgkZTG+SbiGXI1E=; b=d0MBuScaUl4wt4y9JDCj06USlGVGf03v/lJySv6lZJLo1X7swt8Kdue6iIYTeR9FTB 2DLbr705nOu8cJwYewXm1yTfADWb2/791dw3Hk5ATGlegW6jbWNIazgVWzYkhYHUKeou kWX/qvnKVB9AuxuBumunUP6D2GQ6QBbMKMzAf7XTjQ/Nw3c8C1TPft5sXdTF+AVNg+lY N2zzvSAIwjyHtjPv8CioRC0Nf5aj/M0qPupvkpRSFivpLpsnWxm5aswhvOmj6GEb88yr lsrUWIcBZlDgz3l7eSAhksyMCvtfOTT30ZsSkboDcg2vtfw2sPK6rovJ+b2IyTAmOWEI qDuQ== X-Gm-Message-State: AC+VfDxTbM1N3PRPNYP4GzxNBtd2aMpk96IF+IrDSUAnW5KZe+Py65QF DQUe/3qAWk/E4SRo6/9N7gyvflNG0o0aoSKPNwc= X-Google-Smtp-Source: ACHHUZ4so1HhAmeVzXy1rme+zAdClCYGYMTQBLc4ToQQwb+B/wHPQZ73dMXX7n3OOeeGE0dvUYFaYg== X-Received: by 2002:a17:903:228e:b0:1b2:4852:9a5f with SMTP id b14-20020a170903228e00b001b248529a5fmr626103plh.54.1686364630818; Fri, 09 Jun 2023 19:37:10 -0700 (PDT) Received: from localhost ([139.177.225.247]) by smtp.gmail.com with ESMTPSA id ix1-20020a170902f80100b001a6b2813c13sm3906248plb.172.2023.06.09.19.37.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 09 Jun 2023 19:37:10 -0700 (PDT) From: Peng He X-Google-Original-From: Peng He To: ovs-dev@openvswitch.org Date: Fri, 9 Jun 2023 15:03:31 +0000 Message-Id: <20230609150331.817774-1-hepeng.0320@bytedance.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Cc: i.maximets@ovn.org Subject: [ovs-dev] [ovs-dev v11] ofproto-dpif-upcall: fix push_dp_ops 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" push_dp_ops only handles delete ops errors but ignores the modify ops results. It's better to handle all the dp operation errors in a consistent way. This patch prevents the inconsistency by considering modify failure in revalidators. To note, we cannot perform two state transitions and change ukey_state into UKEY_EVICTED directly here, because, if we do so, the sweep will remove the ukey alone and leave dp flow alive. Later, the dump will retrieve the dp flow and might even recover it. This will contribute the stats of this dp flow twice. v8->v9: add testsuite and delete INCONSISTENT ukey at revalidate_sweep. v9->v10: change the commit message and refine the test case. v10->v11: fix indentation and refine the test case. Signed-off-by: Peng He --- ofproto/ofproto-dpif-upcall.c | 51 +++++++++++++++++++++++++---------- tests/dpif-netdev.at | 44 ++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 14 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index cd57fdbd9..c920c749c 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -62,6 +62,7 @@ COVERAGE_DEFINE(upcall_flow_limit_hit); COVERAGE_DEFINE(upcall_flow_limit_kill); COVERAGE_DEFINE(upcall_ukey_contention); COVERAGE_DEFINE(upcall_ukey_replace); +COVERAGE_DEFINE(dumped_inconsistent_flow); /* A thread that reads upcalls from dpif, forwards each upcall's packet, * and possibly sets up a kernel flow as a cache. */ @@ -258,6 +259,7 @@ enum ukey_state { UKEY_CREATED = 0, UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is queued. */ UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */ + UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is inconsistent. */ UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued. */ UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */ UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */ @@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum ukey_state dst, * UKEY_VISIBLE -> UKEY_EVICTED * A handler attempts to install the flow, but the datapath rejects it. * Consider that the datapath has already destroyed it. + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT + * A revalidator modifies the flow with error returns. + * UKEY_INCONSISTENT -> UKEY_EVICTING + * A revalidator decides to evict the datapath flow. * UKEY_OPERATIONAL -> UKEY_EVICTING * A revalidator decides to evict the datapath flow. * UKEY_EVICTING -> UKEY_EVICTED @@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum ukey_state dst, * UKEY_EVICTED -> UKEY_DELETED * A revalidator has removed the ukey from the umap and is deleting it. */ - if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE && - dst < UKEY_DELETED)) { + if (ukey->state == dst - 1 || + (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) || + (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) { ukey->state = dst; } else { struct ds ds = DS_EMPTY_INITIALIZER; @@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, size_t n_ops) for (i = 0; i < n_ops; i++) { struct ukey_op *op = &ops[i]; - struct dpif_flow_stats *push, *stats, push_buf; - - stats = op->dop.flow_del.stats; - push = &push_buf; - - if (op->dop.type != DPIF_OP_FLOW_DEL) { - /* Only deleted flows need their stats pushed. */ - continue; - } if (op->dop.error) { - /* flow_del error, 'stats' is unusable. */ if (op->ukey) { ovs_mutex_lock(&op->ukey->mutex); - transition_ukey(op->ukey, UKEY_EVICTED); + if (op->dop.type == DPIF_OP_FLOW_DEL) { + transition_ukey(op->ukey, UKEY_EVICTED); + } else { + /* Modification of the flow failed */ + transition_ukey(op->ukey, UKEY_INCONSISTENT); + } ovs_mutex_unlock(&op->ukey->mutex); } continue; } + if (op->dop.type != DPIF_OP_FLOW_DEL) { + /* Only deleted flows need their stats pushed. */ + continue; + } + + struct dpif_flow_stats *push, *stats, push_buf; + + stats = op->dop.flow_del.stats; + push = &push_buf; + if (op->ukey) { ovs_mutex_lock(&op->ukey->mutex); transition_ukey(op->ukey, UKEY_EVICTED); @@ -2839,6 +2851,16 @@ revalidate(struct revalidator *revalidator) continue; } + if (ukey->state == UKEY_INCONSISTENT) { + ukey->dump_seq = dump_seq; + COVERAGE_INC(dumped_inconsistent_flow); + reval_op_init(&ops[n_ops++], UKEY_DELETE, udpif, ukey, + &recircs, &odp_actions); + ovs_mutex_unlock(&ukey->mutex); + continue; + } + + if (ukey->state <= UKEY_OPERATIONAL) { /* The flow is now confirmed to be in the datapath. */ transition_ukey(ukey, UKEY_OPERATIONAL); @@ -2927,13 +2949,14 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) } ukey_state = ukey->state; if (ukey_state == UKEY_OPERATIONAL + || (ukey_state == UKEY_INCONSISTENT) || (ukey_state == UKEY_VISIBLE && purge)) { struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER; bool seq_mismatch = (ukey->dump_seq != dump_seq && ukey->reval_seq != reval_seq); enum reval_result result; - if (purge) { + if (purge || ukey_state == UKEY_INCONSISTENT) { result = UKEY_DELETE; } else if (!seq_mismatch) { result = UKEY_KEEP; diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at index baab60a22..2d0dc14c1 100644 --- a/tests/dpif-netdev.at +++ b/tests/dpif-netdev.at @@ -716,3 +716,47 @@ AT_CHECK([test `ovs-vsctl get Interface p2 statistics:tx_q0_packets` -gt 0 -a dn OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([dpif-netdev - revalidators handle dp modification fail correctly]) +OVS_VSWITCHD_START( + [add-port br0 p1 \ + -- set interface p1 type=dummy \ + -- set bridge br0 datapath-type=dummy \ + -- add-port br0 p2 \ + -- set interface p2 type=dummy -- + ]) + +ovs-ofctl add-flow br0 'table=0,in_port=p1,actions=p2' +ovs-appctl netdev-dummy/receive p1 'ipv4(src=10.0.0.1,dst=10.0.0.2),tcp(src=1,dst=2)' +ovs-appctl netdev-dummy/receive p1 'ipv4(src=10.0.0.1,dst=10.0.0.2),tcp(src=1,dst=2)' + +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*thread://' | strip_xout_keep_actions ], [0], [ +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:2 +]) + +dnl Wait for the dp flow to enter OPERATIONAL state. +ovs-appctl revalidator/wait + +AT_CHECK([ovs-appctl revalidator/pause]) + +dnl Delete all dp flows, so flow modification will fail. +AT_CHECK([ovs-appctl dpctl/del-flows]) + +AT_CHECK([ovs-appctl revalidator/resume]) + +dnl Replace OpenFlow rules, trigger revalidation and wait for it to complete. +AT_CHECK([echo 'table=0,in_port=p1,ip actions=ct(commit)' | ovs-ofctl --bundle replace-flows br0 -]) +ovs-appctl revalidator/wait + +dnl Inconsistent ukey should be deleted. +AT_CHECK([ovs-appctl upcall/show | grep keys | grep -q -v 0], [1]) + +dnl Check the error log of the flow modification +AT_CHECK([grep -q -E ".*failed to put.*$" ovs-vswitchd.log]) + +dnl Remove warning logs to let testsuite pass. +OVS_VSWITCHD_STOP([dnl +"/.*failed to put.*$/d +/.*failed to flow_del.*$/d" +]) +AT_CLEANUP