From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8D3E9432D3; Mon, 13 Nov 2023 13:05:04 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0EBB8402B7; Mon, 13 Nov 2023 13:05:04 +0100 (CET) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id 121744026C for ; Mon, 13 Nov 2023 13:05:02 +0100 (CET) Received: from debian (unknown [78.109.66.56]) (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 agw.arknetworks.am (Postfix) with ESMTPSA id 732D4E01C9; Mon, 13 Nov 2023 16:05:01 +0400 (+04) DKIM-Filter: OpenDKIM Filter v2.11.0 agw.arknetworks.am 732D4E01C9 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arknetworks.am; s=default; t=1699877101; bh=QTF+BngX8F9Kn5Ge/w+k9/ZyF0o9o7nE28jndFepiHU=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=IKLZ8rDfcn2DwY+pZgzYUn/w+4JktHJJINbbTcSJ5nIWWM6hKr9y3hw2TNuXZG3q6 vUC3E95xML2rDILA+3hShiYyt7KB5+drmEcKAHGNX23CMybcHNwAhmMqbhWJ26ZsKW c8C7uhDQ2+RV+PRNfp5EW0uce88mpYvD5tiwjq39QDh5VX+P/O8XdlJufVBa85B2qS /1O9oZ5TyZU2fBwY7U8IzsVqCF1sHestTCmBvA+QyiiT7k2VyKqt2IKyzMC6WoKuul kvu5CR9x3JO9QXv1iHflqtL3DMGur7V+QpRDggDh2b+cIRu9MpMR6zun3RijEOQC4C 9EuhaKX6ABIFQ== Date: Mon, 13 Nov 2023 16:04:51 +0400 (+04) From: Ivan Malov To: Stephen Hemminger cc: Andrew Rybchenko , dev@dpdk.org Subject: Re: BUILD bug hidden in SFC driver. In-Reply-To: <20231111085634.5df512bf@hermes.local> Message-ID: <97a09c56-d946-8c0b-85e2-c2ea4ce25dd6@arknetworks.am> References: <20231111085634.5df512bf@hermes.local> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323328-180476581-1699877099=:8212" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-180476581-1699877099=:8212 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT Hi Stephen, On Sat, 11 Nov 2023, Stephen Hemminger wrote: > While examining the use of VLA in DPDK, ran into a bug in sfc driver. > > If DPDK is built with -Wvla, then the RTE_BUILD_BUG_ON() macro won't work > as written. Experimenting with a better more portable version of that macro > as: > #define RTE_BUILD_BUG_ON(e) _Static_assert(!(e), #e) First of all, thanks for the effort. Very helpful. Please see below. > > revealed that the SFC driver was calling RTE_BUILD_BUG_ON with non constant > expression. > > ../drivers/net/sfc/sfc_ef100_tx.c: In function ‘sfc_ef100_tx_pkt_descs_max’: > ../lib/eal/include/rte_common.h:585:20: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare] > 585 | _a < _b ? _a : _b; \ > | ^ > ../lib/eal/include/rte_common.h:498:46: note: in definition of macro ‘RTE_BUILD_BUG_ON’ > 498 | #define RTE_BUILD_BUG_ON(e) _Static_assert(!(e), #e) > | ^ > ../drivers/net/sfc/sfc_ef100_tx.c:566:34: note: in expansion of macro ‘RTE_MIN’ > 566 | RTE_MIN((unsigned int)EFX_MAC_PDU_MAX, > | ^~~~~~~ > ../lib/eal/include/rte_common.h:585:32: warning: operand of ‘?:’ changes signedness from ‘int’ to ‘unsigned int’ due to unsignedness of other operand [-Wsign-compare] > 585 | _a < _b ? _a : _b; \ > | ^~ > ../lib/eal/include/rte_common.h:498:46: note: in definition of macro ‘RTE_BUILD_BUG_ON’ > 498 | #define RTE_BUILD_BUG_ON(e) _Static_assert(!(e), #e) > | ^ > ../drivers/net/sfc/sfc_ef100_tx.c:566:34: note: in expansion of macro ‘RTE_MIN’ > 566 | RTE_MIN((unsigned int)EFX_MAC_PDU_MAX, > | ^~~~~~~ > ../lib/eal/include/rte_common.h:498:44: error: expression in static assertion is not constant > 498 | #define RTE_BUILD_BUG_ON(e) _Static_assert(!(e), #e) > | ^~~~ > ../drivers/net/sfc/sfc_ef100_tx.c:565:17: note: in expansion of macro ‘RTE_BUILD_BUG_ON’ > > > The problem is that Gcc does not evaluate a ternary operator expression > with all constants at compile time to produce a constant value! Apparently, > the language standards leave this as ambiguous. > > If the code is expanded into two assertions as: > > diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c > index 1b6374775f07..25e6633d6679 100644 > --- a/drivers/net/sfc/sfc_ef100_tx.c > +++ b/drivers/net/sfc/sfc_ef100_tx.c > @@ -562,9 +562,8 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m) > * Make sure that the first segment does not need fragmentation > * (split into many Tx descriptors). > */ > - RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX < > - RTE_MIN((unsigned int)EFX_MAC_PDU_MAX, > - SFC_MBUF_SEG_LEN_MAX)); > + RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX < EFX_MAC_PDU_MAX); > + RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX < SFC_MBUF_SEG_LEN_MAX); > } > > if (m->ol_flags & sfc_dp_mport_override) { > > Then a new problem arises: > In file included from ../lib/mbuf/rte_mbuf.h:36, > from ../drivers/net/sfc/sfc_ef100_tx.c:12: > ../drivers/net/sfc/sfc_ef100_tx.c: In function ‘sfc_ef100_tx_pkt_descs_max’: > ../lib/eal/include/rte_common.h:498:29: error: static assertion failed: "SFC_EF100_TX_SEND_DESC_LEN_MAX < SFC_MBUF_SEG_LEN_MAX" > 498 | #define RTE_BUILD_BUG_ON(e) _Static_assert(!(e), #e) > | ^~~~~~~~~~~~~~ > ../drivers/net/sfc/sfc_ef100_tx.c:566:17: note: in expansion of macro ‘RTE_BUILD_BUG_ON’ > 566 | RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX < SFC_MBUF_SEG_LEN_MAX); > | ^~~~~~~~~~~~~~~~ > > Building a little program to unwind the #defines reveals: > > SFC_EF100_TX_SEND_DESC_LEN_MAX = 16383 > EFX_MAC_PDU_MAX = 9240 > SFC_MBUF_SEG_LEN_MAX = 65535 > > I.e: > RTE_BUILD_BUG_ON(16383 < RTE_MIN(9240, 65535)); > > > Therefore the current driver should be getting build bug, but the existing macro > hides it. As far as I understand, the intention behind this check is to make sure that SFC_EF100_TX_SEND_DESC_LEN_MAX represents enough room to accommodate either EFX_MAC_PDU_MAX or SFC_MBUF_SEG_LEN_MAX bytes, whichever is smaller. Is 16383 sufficient to accommodate 9240? I think so. Do you agree? That being said, indeed, applying the "more portable version" of yours results in me seeing the warning about a non-constant expression. Applying the following patch makes all errors disappear when building with either version of RTE_BUILD_BUG_ON: diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c index 1b6374775f..01f37c2616 100644 --- a/drivers/net/sfc/sfc_ef100_tx.c +++ b/drivers/net/sfc/sfc_ef100_tx.c @@ -563,7 +563,7 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m) * (split into many Tx descriptors). */ RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX < - RTE_MIN((unsigned int)EFX_MAC_PDU_MAX, + MIN((unsigned int)EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX)); } with MIN being defined in drivers/common/sfc_efx/efsys.h as #define MIN(v1, v2) ((v1) < (v2) ? (v1) : (v2)) Would that be an acceptable fix? Or am I missing something? Thank you. --8323328-180476581-1699877099=:8212--