From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id 767E2CD4F54 for ; Wed, 27 May 2026 11:07:10 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8094940395; Wed, 27 May 2026 13:07:08 +0200 (CEST) Received: from smtpbgeu1.qq.com (smtpbgeu1.qq.com [52.59.177.22]) by mails.dpdk.org (Postfix) with ESMTP id 404C440658 for ; Wed, 27 May 2026 13:07:06 +0200 (CEST) X-QQ-mid: Yeas4t1779880021t527t39416 Received: from 0F57A7141CBF4D1588B97A6ED8A17143 (zaiyuwang@trustnetic.com [122.231.28.113]) X-QQ-SSF: 0000000000000000000000000000000 From: =?utf-8?b?WmFpeXUgV2FuZw==?= X-BIZMAIL-ID: 9690366586208227561 To: "'Stephen Hemminger'" Cc: References: <20260423034024.14404-1-zaiyuwang@trustnetic.com> <20260511103604.19724-1-zaiyuwang@trustnetic.com> <20260511103604.19724-8-zaiyuwang@trustnetic.com> <20260517164444.7c0cb0e0@phoenix.local> In-Reply-To: <20260517164444.7c0cb0e0@phoenix.local> Subject: RE: [PATCH v4 07/20] net/txgbe: fix Tx desc free logic Date: Wed, 27 May 2026 19:06:59 +0800 Message-ID: <00bb01dcedc8$f100dd00$d3029700$@trustnetic.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQIzGrJ934oa1ka+Tuv/tXGR6pozcwG6Z/2aAh6WUEsCIHykxLU79VtA Content-Language: zh-cn X-QQ-SENDSIZE: 520 Feedback-ID: Yeas:trustnetic.com:qybglogicsvrsz:qybglogicsvrsz3b-0 X-QQ-XMAILINFO: MusMsqF/YqAw2dW2xdS4CtW+pfggW21rEt7kFQDeLaWWIikqOhSkHIEr lA27u6mbOBBMRuqFLKaX/lhuoisHkaehUXc4SKEHVxpAagmw1Y2r9jf59KSMXXethXg8ZYx PyNLcVJ5OgsD6orbYBb2sg27Pwbtx8tompIg5yPJhG/2qJs9eX4kwU8k2hSlBQZTpXa3rI6 DQEW9K7GhseUP9chIxidpQbXj4sa295EZQHeqM3udQBtYBu53oe89t0VPyeWetYnq7Ek72V IshJGOLqgQwyIBj1S3OjrbEsry70gRvsOusr9QPJKOAMnn0tCW0FU78oaVYwXC7Pgl6bH1B 6x/Z9UIbtBlsvqxaUKdqNlJpyLVVkGKUkQOdENQCU83YPbMi6isQcS2ell13StR0DYWljwI WhGc1LLhV1xP+97CryWYLP1FluqtbF4/5CySpgJ58ZQ6blgOxkDpw/3iRn1MrSslnfwX60Z 1m0vrzLVc3WW/HOp8OC9+BoY58fN3dXW3eaaoG9SLlxCmHdG71hjd1nD3/SOY90wKRKiXB9 f/zzSbrMaZ4wEb+pwQeL7c8nITKfbgALcqOn5KaVWFfs/y1lSsxNVBqlCeVUt9lIcSK2fjp 1vDOVjwLwpNqAItmOmgWPLUUoqrTOT+cTjoWwhxKO7os3KefidEqCEka3y/tbs9yYTN8S4N UWG4nuJVMlEq0Y8ceEGk0TuO5bUQ1PyS+XOMi3DP4Ai6R5u6khHprjXq68SM7Kb4CaWX4Wv n5+eTGIUymFedngBdGssdVeAp60s7ICpwUc782IqCg2bBynsok1DpsEk8b6IxMvs+G24295 IgmUq5aYSmqB19W3o9BtTS0rcElrDMuPnosmeBV3J+4hgvUQW4ZXAkF6dAGbOfyTRnax6jE EjihUpFm+97lhTAZWDDDUFBq5SZliHugkkOU7pCIGh6vmXQQwJwuqYiq3mg36BxeiNtIisD 2bg4o2/3YQ0ezMzmItB0KAXmYSDXCZqIqG4ws5qAL96TlSCgKqTHzWMn6skRV51GoRhL3Wt Jk8zS57g== X-QQ-XMRINFO: M/715EihBoGS47X28/vv4NpnfpeBLnr4Qg== X-QQ-RECHKSPAM: 0 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 > Lots of AI feedback on this one. > > > # Review of DPDK Patch: net/txgbe: fix Tx desc free logic > > ## Errors > > ### 1. Shared variable access without atomics (3 locations) > > **Lines affected:** > - `drivers/net/txgbe/txgbe_rxtx.c:101` (txgbe_tx_free_bufs) > - `drivers/net/txgbe/txgbe_rxtx.c:655` (txgbe_xmit_cleanup) > - `drivers/net/txgbe/txgbe_rxtx_vec_common.h:98` (txgbe_tx_free_bufs) > > ```c > volatile uint16_t head = (uint16_t)*txq->headwb_mem; ``` > > **Problem:** The code reads a hardware-updated value (`*txq->headwb_mem`) > using a `volatile` qualifier. The `volatile` keyword does NOT provide atomicity or > memory ordering guarantees between the hardware and CPU. On some > architectures, reading a `uint16_t` from DMA-accessible memory without proper > barriers or atomic operations can result in torn reads, stale cached values, or > reordering issues. > > **Fix:** Use atomic load with appropriate memory ordering: > > ```c > uint16_t head = rte_atomic_load_explicit( > (uint16_t *)txq->headwb_mem, > rte_memory_order_acquire); > ``` > > The `acquire` ordering ensures that all descriptor writes by the hardware are visible > to the CPU after reading the head pointer. This is critical for correctness in > hardware write-back scenarios. > > Note: The `headwb_mem` field type may need adjustment to be an `rte_atomic` > type or cast appropriately. > Fixed. All three reads from headwb_mem now use rte_atomic_load_explicit with rte_memory_order_acquire, and the loaded value is declared const. > --- > > ### 2. Whitespace/indentation error in vec_common.h > > **Line:** `drivers/net/txgbe/txgbe_rxtx_vec_common.h:97-98` > > ```c > tx_last_dd -= txq->nb_tx_desc; > > volatile uint16_t head = (uint16_t)*txq->headwb_mem; ``` > > **Problem:** Inconsistent indentation. The `volatile uint16_t head` declaration is > indented with tabs-only (correct), but it visually appears to be part of a removed > block based on the diff context showing the previous line had different > indentation. > > **Fix:** Verify the indentation matches the surrounding code block. Based on the > diff, this appears correct after the change, but confirm the tabs align with the > function's indentation level. Verified, the indentation is correct and matches the surrounding code. > > --- > > ## Warnings > > ### 1. Logic change correctness verification needed > > The patch adds three new early-return conditions checking if `head == tx_next_dd`. > While this appears correct (hardware hasn't advanced past the point we want to > free to), the logic for wrap-around cases should be verified: > > ```c > if (txq->tx_next_dd == head) > return 0; > else if (txq->tx_next_dd > head && head > tx_last_dd) > return 0; > else if (tx_last_dd > txq->tx_next_dd && > (head > tx_last_dd || head < txq->tx_next_dd)) > return 0; > ``` > > **Concern:** The wrap-around logic assumes the ring wraps at `nb_tx_desc`. The > condition `tx_last_dd > txq->tx_next_dd` handles the wrap case, but verify that: > - `tx_last_dd` calculation correctly wraps (it does: `tx_last_dd -= txq->nb_tx_desc`) > - The inequality checks are strict `>` not `>=` consistently > > The existing code appears correct, but this is complex boundary logic that warrants > testing, especially the second `else if` condition in wrap-around scenarios. The logic has been carefully reviewed and tested. The extracted helper function preserves the exact same conditions as the original code, so no behavioural change is introduced. > > --- > > ### 2. Missing const qualifier on read-only atomic load result > > **All three locations:** The `head` variable is read from hardware but never > modified: > > ```c > volatile uint16_t head = (uint16_t)*txq->headwb_mem; ``` > > **Suggestion:** Once converted to atomic operations, declare as `const`: > > ```c > const uint16_t head = rte_atomic_load_explicit(...); ``` > > This documents that `head` is never modified after the initial read and prevents > accidental writes. Done, the loaded head value is now const. > > --- > > ## Info > > ### 1. Consider helper function for repeated logic > > The same boundary-check logic appears in three separate functions with only > minor variations. Consider extracting it to a static inline helper: > > ```c > static inline bool > txgbe_hw_head_in_range(uint16_t hw_head, uint16_t start, > uint16_t end, uint16_t ring_size) { > if (start == hw_head) > return false; > > if (start > hw_head) > return hw_head <= end; > else /* wrap case */ > return hw_head > end || hw_head < start; } ``` > > This would reduce code duplication and make the logic easier to test and maintain. I have extracted the common boundary-check logic into a new static inline helper, txgbe_tx_headwb_desc_done(), placed in txgbe_rxtx.h. All three call sites now use this helper. > > --- > > ### 2. Comment explaining write-back head semantics > > The commit message mentions "Tx head write-back mode" but the code lacks > comments explaining the hardware behavior. Consider adding: > > ```c > /* > * In Amber-Lite NIC Tx head write-back mode, hardware periodically > * updates *headwb_mem with the index of the next descriptor to process. > * Descriptors before this head pointer are safe to free. > */ > const uint16_t head = rte_atomic_load_explicit(...); ``` Added a detailed comment for the helper function explaining the hardware behavior and the (last, next] interval semantics. > > --- > > ## Summary > > **Must fix (Errors):** > 1. Replace `volatile` read with `rte_atomic_load_explicit()` with > `rte_memory_order_acquire` ordering in all three locations > > **Should fix (Warnings):** > 1. Add `const` qualifier to the loaded `head` value after converting to atomics 2. > Verify wrap-around boundary logic with testing (appears correct but is complex) > > **Consider (Info):** > 1. Extract repeated boundary-check logic to a helper function 2. Add comments > documenting hardware write-back semantics > > The core fix (adding the `head == tx_next_dd` check) appears correct and > addresses the described bug. The primary issue is the use of `volatile` instead of > proper atomic operations for accessing hardware-updated shared memory. > >