| Message ID | 20230509170935.2237051-2-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Intel Wired LAN Driver Updates 2023-05-09 (igc, igb) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, 9 May 2023 10:09:33 -0700 Tony Nguyen wrote: > There could be a race condition during link down where interrupt > being generated and igc_clean_tx_irq() been called to perform the > TX completion. Properly clear the TX buffer and TX descriptor ring > to avoid those case. > + /* Zero out the buffer ring */ > + memset(tx_ring->tx_buffer_info, 0, > + sizeof(*tx_ring->tx_buffer_info) * tx_ring->count); > + > + /* Zero out the descriptor ring */ > + memset(tx_ring->desc, 0, tx_ring->size); Just from the diff and the commit description this does not seem obviously correct. Race condition means the two functions can run at the same time, and memset() is not atomic.
Hi Jakub, > On Tue, 9 May 2023 10:09:33 -0700 Tony Nguyen wrote: > > There could be a race condition during link down where interrupt being > > generated and igc_clean_tx_irq() been called to perform the TX > > completion. Properly clear the TX buffer and TX descriptor ring to > > avoid those case. > > > + /* Zero out the buffer ring */ > > + memset(tx_ring->tx_buffer_info, 0, > > + sizeof(*tx_ring->tx_buffer_info) * tx_ring->count); > > + > > + /* Zero out the descriptor ring */ > > + memset(tx_ring->desc, 0, tx_ring->size); > > Just from the diff and the commit description this does not seem obviously > correct. Race condition means the two functions can run at the same time, > and memset() is not atomic. While a link is going up or down and a lot of packets(UDP) are being sent transmitted, we are observing some kernel panic issues. On my side, it was easily to reproduce. It's possible that igc_clean_tx_irq() was called to complete the TX during link up/down based on how the call trace looks. With this fix, I not observed the issue anymore. Almost similar issue reported before in here: https://lore.kernel.org/all/SJ1PR11MB6180CDB866753CFBC2F9AF75B8959@SJ1PR11MB6180.namprd11.prod.outlook.com/ > -- > pw-bot: cr
On Fri, May 12, 2023 at 08:51:23AM +0000, Zulkifli, Muhammad Husaini wrote: > Hi Jakub, > > > On Tue, 9 May 2023 10:09:33 -0700 Tony Nguyen wrote: > > > There could be a race condition during link down where interrupt being > > > generated and igc_clean_tx_irq() been called to perform the TX > > > completion. Properly clear the TX buffer and TX descriptor ring to > > > avoid those case. > > > > > + /* Zero out the buffer ring */ > > > + memset(tx_ring->tx_buffer_info, 0, > > > + sizeof(*tx_ring->tx_buffer_info) * tx_ring->count); > > > + > > > + /* Zero out the descriptor ring */ > > > + memset(tx_ring->desc, 0, tx_ring->size); > > > > Just from the diff and the commit description this does not seem obviously > > correct. Race condition means the two functions can run at the same time, > > and memset() is not atomic. > > While a link is going up or down and a lot of packets(UDP) are being sent transmitted, > we are observing some kernel panic issues. On my side, it was easily to reproduce. > It's possible that igc_clean_tx_irq() was called to complete the TX during link up/down > based on how the call trace looks. With this fix, I not observed the issue anymore. then include the splat you were getting in the commit msg as well as steps to repro. from a brief look it looks like ndo_stop() path does not disable Tx rings before cleaning them? This is being done when configuring xsk_pool on a given Tx ring, though. > > Almost similar issue reported before in here: > https://lore.kernel.org/all/SJ1PR11MB6180CDB866753CFBC2F9AF75B8959@SJ1PR11MB6180.namprd11.prod.outlook.com/ > > > -- > > pw-bot: cr >
Hi Maciej, > On Fri, May 12, 2023 at 08:51:23AM +0000, Zulkifli, Muhammad Husaini > wrote: > > Hi Jakub, > > > > > On Tue, 9 May 2023 10:09:33 -0700 Tony Nguyen wrote: > > > > There could be a race condition during link down where interrupt > > > > being generated and igc_clean_tx_irq() been called to perform the > > > > TX completion. Properly clear the TX buffer and TX descriptor ring > > > > to avoid those case. > > > > > > > + /* Zero out the buffer ring */ > > > > + memset(tx_ring->tx_buffer_info, 0, > > > > + sizeof(*tx_ring->tx_buffer_info) * tx_ring->count); > > > > + > > > > + /* Zero out the descriptor ring */ > > > > + memset(tx_ring->desc, 0, tx_ring->size); > > > > > > Just from the diff and the commit description this does not seem > > > obviously correct. Race condition means the two functions can run at > > > the same time, and memset() is not atomic. > > > > While a link is going up or down and a lot of packets(UDP) are being > > sent transmitted, we are observing some kernel panic issues. On my side, it > was easily to reproduce. > > It's possible that igc_clean_tx_irq() was called to complete the TX > > during link up/down based on how the call trace looks. With this fix, I not > observed the issue anymore. > > then include the splat you were getting in the commit msg as well as steps to > repro. > > from a brief look it looks like ndo_stop() path does not disable Tx rings before > cleaning them? This is being done when configuring xsk_pool on a given Tx ring, > though. Yes you are right. We shall disable tx queue ring as well. I will submit the V2 to replace the igc_clean_tx_ring() to igc_disable_tx_ring() in igc_free_tx_resources() during ndo_stop callback while maintaining the memset to clear all the ring des/buffer during igc_clean_tx_ring(). I have tested this on my TGL setup with multiple loops iterations and no more Kernel panic observed. Will update this in commit message as well Thanks, Husaini > > > > > Almost similar issue reported before in here: > > > https://lore.kernel.org/all/SJ1PR11MB6180CDB866753CFBC2F9AF75B8959@S > J1 > > PR11MB6180.namprd11.prod.outlook.com/ > > > > > -- > > > pw-bot: cr > >
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 1c4676882082..5767e691b401 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -254,6 +254,13 @@ static void igc_clean_tx_ring(struct igc_ring *tx_ring) /* reset BQL for queue */ netdev_tx_reset_queue(txring_txq(tx_ring)); + /* Zero out the buffer ring */ + memset(tx_ring->tx_buffer_info, 0, + sizeof(*tx_ring->tx_buffer_info) * tx_ring->count); + + /* Zero out the descriptor ring */ + memset(tx_ring->desc, 0, tx_ring->size); + /* reset next_to_use and next_to_clean */ tx_ring->next_to_use = 0; tx_ring->next_to_clean = 0;