From patchwork Tue Aug 11 06:15:10 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1343167 X-Patchwork-Delegate: i.maximets@samsung.com 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.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BQjHl0g4qz9sTT for ; Tue, 11 Aug 2020 16:15:29 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 00550876B8; Tue, 11 Aug 2020 06:15:26 +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 9QpYmY0qAZhc; Tue, 11 Aug 2020 06:15:25 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 4A04687601; Tue, 11 Aug 2020 06:15:25 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 27FD6C013C; Tue, 11 Aug 2020 06:15:25 +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 36672C004D for ; Tue, 11 Aug 2020 06:15:24 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 1CD4287601 for ; Tue, 11 Aug 2020 06:15:24 +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 6ThPuKN7BP+0 for ; Tue, 11 Aug 2020 06:15:23 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by whitealder.osuosl.org (Postfix) with ESMTPS id BDD4B875FE for ; Tue, 11 Aug 2020 06:15:22 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [216.113.160.71]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 644FF10000A; Tue, 11 Aug 2020 06:15:18 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Mon, 10 Aug 2020 23:15:10 -0700 Message-Id: <1597126510-78635-1-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 Cc: Han Zhou Subject: [ovs-dev] [PATCH] Revert "ovsdb-idl: Fix NULL deref reported by Coverity." 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" This reverts commit 68bc6f88a3a36549fcd3b6248c25c5e2e6deb8f3. The commit causes a regression in OVN scale test. ovn-northd's CPU more than doubled for the test scenario: create and bind 12k ports. Below are some perf data of ovn-northd when running command: ovn-nbctl --wait=sb sync Before reverting this commit: - 92.42% 0.62% ovn-northd ovn-northd [.] main - 91.80% main + 68.93% ovn_db_run (inlined) + 22.45% ovsdb_idl_loop_commit_and_wait After reverting this commit: - 92.84% 0.60% ovn-northd ovn-northd [.] main - 92.24% main + 92.03% ovn_db_run (inlined) Reverting this commit avoided 22.45% of the CPU caused by ovsdb_idl_loop_commit_and_wait(). The commit changed the logic of ovsdb_idl_txn_write__() by adding the check "datum->keys && datum->values" before discarding unchanged data in a transaction. However, it is normal for OVSDB clients ( such as ovn-northd) to try to set columns with same empty data as it is before the transaction. IDL would discard these changes and avoid sending big transactions to server (which would end up as no-op on server side). In the ovn scale test scenario mentioned above, each iteration of ovn-northd would send a transaction to server that includes all rows of the huge Port_Binding table, which caused the significant CPU increase of ovn-northd (and also the OVN SB DB server), resulted in longer end to end latency of OVN configuration changes. For the original problem the commit 68bc6f88 was trying to fix, it doesn't seem to be a real problem. The NULL deref reported by Coverity may be addressed in a future patch using a different approach, if necessary. Cc: William Tu Signed-off-by: Han Zhou --- lib/ovsdb-idl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index ef3b97b..d8f221c 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -4631,8 +4631,7 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_, * transaction only does writes of existing values, without making any real * changes, we will drop the whole transaction later in * ovsdb_idl_txn_commit().) */ - if (datum->keys && datum->values && - write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), + if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), datum, &column->type)) { goto discard_datum; }