From: Ivan Malov <ivan.malov@arknetworks.am>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>, dev@dpdk.org
Subject: Re: BUILD bug hidden in SFC driver.
Date: Mon, 13 Nov 2023 16:04:51 +0400 (+04) [thread overview]
Message-ID: <97a09c56-d946-8c0b-85e2-c2ea4ce25dd6@arknetworks.am> (raw)
In-Reply-To: <20231111085634.5df512bf@hermes.local>
[-- Attachment #1: Type: text/plain, Size: 5770 bytes --]
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.
next prev parent reply other threads:[~2023-11-13 12:05 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-11 16:56 Stephen Hemminger
2023-11-11 17:29 ` Stephen Hemminger
2023-11-13 12:04 ` Ivan Malov [this message]
2023-11-13 16:30 ` Stephen Hemminger
2023-11-13 16:28 ` Tyler Retzlaff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=97a09c56-d946-8c0b-85e2-c2ea4ce25dd6@arknetworks.am \
--to=ivan.malov@arknetworks.am \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).