We found some bugs when testing the XDP function of enetc driver, and these bugs are easy to reproduce. This is not only causes XDP to not work, but also the network cannot be restored after exiting the XDP program. So the patch set is mainly to fix these bugs. For details, please see the commit message of each patch.
Wei Fang (3): net: enetc: remove xdp_drops statistic from enetc_xdp_drop() net: enetc: fix the issues of XDP_REDIRECT feature net: enetc: reset xdp_tx_in_flight when updating bpf program
drivers/net/ethernet/freescale/enetc/enetc.c | 46 +++++++++++++++----- drivers/net/ethernet/freescale/enetc/enetc.h | 1 + 2 files changed, 37 insertions(+), 10 deletions(-)
The xdp_drops statistic indicates the number of XDP frames dropped in the Rx direction. However, enetc_xdp_drop() is also used in XDP_TX and XDP_REDIRECT actions. If frame loss occurs in these two actions, the frames loss count should not be included in xdp_drops, because there are already xdp_tx_drops and xdp_redirect_failures to count the frame loss of these two actions, so it's better to remove xdp_drops statistic from enetc_xdp_drop() and increase xdp_drops in XDP_DROP action.
Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX") Signed-off-by: Wei Fang wei.fang@nxp.com --- drivers/net/ethernet/freescale/enetc/enetc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 032d8eadd003..56e59721ec7d 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -1521,7 +1521,6 @@ static void enetc_xdp_drop(struct enetc_bdr *rx_ring, int rx_ring_first, &rx_ring->rx_swbd[rx_ring_first]); enetc_bdr_idx_inc(rx_ring, &rx_ring_first); } - rx_ring->stats.xdp_drops++; }
static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring, @@ -1586,6 +1585,7 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring, fallthrough; case XDP_DROP: enetc_xdp_drop(rx_ring, orig_i, i); + rx_ring->stats.xdp_drops++; break; case XDP_PASS: rxbd = orig_rxbd;
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH net 1/3] net: enetc: remove xdp_drops statistic from enetc_xdp_drop() Link: https://lore.kernel.org/stable/20240919084104.661180-2-wei.fang%40nxp.com
When testing the XDP_REDIRECT function on the LS1028A platform, we found a very reproducible issue that the Tx frames can no longer be sent out even if XDP_REDIRECT is turned off. Specifically, if there is a lot of traffic on Rx direction, when XDP_REDIRECT is turned on, the console may display some warnings like "timeout for tx ring #6 clear", and all redirected frames will be dropped, the detaild log is as follows.
root@ls1028ardb:~# ./xdp-bench redirect eno0 eno2 Redirecting from eno0 (ifindex 3; driver fsl_enetc) to eno2 (ifindex 4; driver fsl_enetc) [203.849809] fsl_enetc 0000:00:00.2 eno2: timeout for tx ring #5 clear [204.006051] fsl_enetc 0000:00:00.2 eno2: timeout for tx ring #6 clear [204.161944] fsl_enetc 0000:00:00.2 eno2: timeout for tx ring #7 clear eno0->eno2 1420505 rx/s 1420590 err,drop/s 0 xmit/s xmit eno0->eno2 0 xmit/s 1420590 drop/s 0 drv_err/s 15.71 bulk-avg eno0->eno2 1420484 rx/s 1420485 err,drop/s 0 xmit/s xmit eno0->eno2 0 xmit/s 1420485 drop/s 0 drv_err/s 15.71 bulk-avg
By analyzing the XDP_REDIRECT implementation of enetc driver, we found two problems. First, enetc driver will reconfigure Tx and Rx BD rings when a bpf program is installed or uninstalled, but there is no mechanisms to block the redirected frames when enetc driver reconfigures BD rings. So introduce ENETC_TX_DOWN flag to prevent the redirected frames to be attached to Tx BD rings.
Second, Tx BD rings are disabled first in enetc_stop() and then wait for empty. This operation is not safe while the Tx BD ring is actively transmitting frames, and will cause the ring to not be empty and hardware exception. As described in the block guide of LS1028A NETC, software should only disable an active ring after all pending ring entries have been consumed (i.e. when PI = CI). Disabling a transmit ring that is actively processing BDs risks a HW-SW race hazard whereby a hardware resource becomes assigned to work on one or more ring entries only to have those entries be removed due to the ring becoming disabled. So the correct behavior is that the software stops putting frames on the Tx BD rings (this is what ENETC_TX_DOWN does), then waits for the Tx BD rings to be empty, and finally disables the Tx BD rings.
Fixes: c33bfaf91c4c ("net: enetc: set up XDP program under enetc_reconfigure()") Cc: stable@vger.kernel.org Signed-off-by: Wei Fang wei.fang@nxp.com --- drivers/net/ethernet/freescale/enetc/enetc.c | 43 ++++++++++++++++---- drivers/net/ethernet/freescale/enetc/enetc.h | 1 + 2 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 56e59721ec7d..5830c046cb7d 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -902,6 +902,7 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
if (unlikely(tx_frm_cnt && netif_carrier_ok(ndev) && __netif_subqueue_stopped(ndev, tx_ring->index) && + !test_bit(ENETC_TX_DOWN, &priv->flags) && (enetc_bd_unused(tx_ring) >= ENETC_TXBDS_MAX_NEEDED))) { netif_wake_subqueue(ndev, tx_ring->index); } @@ -1377,6 +1378,9 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames, int xdp_tx_bd_cnt, i, k; int xdp_tx_frm_cnt = 0;
+ if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags))) + return -ENETDOWN; + enetc_lock_mdio();
tx_ring = priv->xdp_tx_ring[smp_processor_id()]; @@ -2223,18 +2227,24 @@ static void enetc_enable_rxbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring) enetc_rxbdr_wr(hw, idx, ENETC_RBMR, rbmr); }
-static void enetc_enable_bdrs(struct enetc_ndev_priv *priv) +static void enetc_enable_rx_bdrs(struct enetc_ndev_priv *priv) { struct enetc_hw *hw = &priv->si->hw; int i;
- for (i = 0; i < priv->num_tx_rings; i++) - enetc_enable_txbdr(hw, priv->tx_ring[i]); - for (i = 0; i < priv->num_rx_rings; i++) enetc_enable_rxbdr(hw, priv->rx_ring[i]); }
+static void enetc_enable_tx_bdrs(struct enetc_ndev_priv *priv) +{ + struct enetc_hw *hw = &priv->si->hw; + int i; + + for (i = 0; i < priv->num_tx_rings; i++) + enetc_enable_txbdr(hw, priv->tx_ring[i]); +} + static void enetc_disable_rxbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring) { int idx = rx_ring->index; @@ -2251,7 +2261,16 @@ static void enetc_disable_txbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring) enetc_txbdr_wr(hw, idx, ENETC_TBMR, 0); }
-static void enetc_disable_bdrs(struct enetc_ndev_priv *priv) +static void enetc_disable_rx_bdrs(struct enetc_ndev_priv *priv) +{ + struct enetc_hw *hw = &priv->si->hw; + int i; + + for (i = 0; i < priv->num_rx_rings; i++) + enetc_disable_rxbdr(hw, priv->rx_ring[i]); +} + +static void enetc_disable_tx_bdrs(struct enetc_ndev_priv *priv) { struct enetc_hw *hw = &priv->si->hw; int i; @@ -2259,8 +2278,6 @@ static void enetc_disable_bdrs(struct enetc_ndev_priv *priv) for (i = 0; i < priv->num_tx_rings; i++) enetc_disable_txbdr(hw, priv->tx_ring[i]);
- for (i = 0; i < priv->num_rx_rings; i++) - enetc_disable_rxbdr(hw, priv->rx_ring[i]); }
static void enetc_wait_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring) @@ -2452,6 +2469,8 @@ void enetc_start(struct net_device *ndev)
enetc_setup_interrupts(priv);
+ enetc_enable_tx_bdrs(priv); + for (i = 0; i < priv->bdr_int_num; i++) { int irq = pci_irq_vector(priv->si->pdev, ENETC_BDR_INT_BASE_IDX + i); @@ -2460,9 +2479,11 @@ void enetc_start(struct net_device *ndev) enable_irq(irq); }
- enetc_enable_bdrs(priv); + enetc_enable_rx_bdrs(priv);
netif_tx_start_all_queues(ndev); + + clear_bit(ENETC_TX_DOWN, &priv->flags); } EXPORT_SYMBOL_GPL(enetc_start);
@@ -2520,9 +2541,11 @@ void enetc_stop(struct net_device *ndev) struct enetc_ndev_priv *priv = netdev_priv(ndev); int i;
+ set_bit(ENETC_TX_DOWN, &priv->flags); + netif_tx_stop_all_queues(ndev);
- enetc_disable_bdrs(priv); + enetc_disable_rx_bdrs(priv);
for (i = 0; i < priv->bdr_int_num; i++) { int irq = pci_irq_vector(priv->si->pdev, @@ -2535,6 +2558,8 @@ void enetc_stop(struct net_device *ndev)
enetc_wait_bdrs(priv);
+ enetc_disable_tx_bdrs(priv); + enetc_clear_interrupts(priv); } EXPORT_SYMBOL_GPL(enetc_stop); diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h index 97524dfa234c..fb7d98d57783 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.h +++ b/drivers/net/ethernet/freescale/enetc/enetc.h @@ -325,6 +325,7 @@ enum enetc_active_offloads {
enum enetc_flags_bit { ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS = 0, + ENETC_TX_DOWN, };
/* interrupt coalescing modes */
When running "xdp-bench tx eno0" to test the XDP_TX feature of ENETC on LS1028A, it was found that if the command was re-run multiple times, Rx could not receive the frames, and the result of xdo-bench showed that the rx rate was 0.
root@ls1028ardb:~# ./xdp-bench tx eno0 Hairpinning (XDP_TX) packets on eno0 (ifindex 3; driver fsl_enetc) Summary 2046 rx/s 0 err,drop/s Summary 0 rx/s 0 err,drop/s Summary 0 rx/s 0 err,drop/s Summary 0 rx/s 0 err,drop/s
By observing the Rx PIR and CIR registers, we found that CIR is always equal to 0x7FF and PIR is always 0x7FE, which means that the Rx ring is full and can no longer accommodate other Rx frames. Therefore, it is obvious that the RX BD ring has not been cleaned up.
Further analysis of the code revealed that the Rx BD ring will only be cleaned if the "cleaned_cnt > xdp_tx_in_flight" condition is met. Therefore, some debug logs were added to the driver and the current values of cleaned_cnt and xdp_tx_in_flight were printed when the Rx BD ring was full. The logs are as follows.
[ 178.762419] [XDP TX] >> cleaned_cnt:1728, xdp_tx_in_flight:2140 [ 178.771387] [XDP TX] >> cleaned_cnt:1941, xdp_tx_in_flight:2110 [ 178.776058] [XDP TX] >> cleaned_cnt:1792, xdp_tx_in_flight:2110
From the results, we can see that the maximum value of xdp_tx_in_flight has reached 2140. However, the size of the Rx BD ring is only 2048. This is incredible, so checked the code again and found that the driver did not reset xdp_tx_in_flight when installing or uninstalling bpf program, resulting in xdp_tx_in_flight still retaining the value after the last command was run.
Fixes: c33bfaf91c4c ("net: enetc: set up XDP program under enetc_reconfigure()") Cc: stable@vger.kernel.org Signed-off-by: Wei Fang wei.fang@nxp.com --- drivers/net/ethernet/freescale/enetc/enetc.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 5830c046cb7d..3cff76923ab9 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -2769,6 +2769,7 @@ static int enetc_reconfigure_xdp_cb(struct enetc_ndev_priv *priv, void *ctx) for (i = 0; i < priv->num_rx_rings; i++) { struct enetc_bdr *rx_ring = priv->rx_ring[i];
+ rx_ring->xdp.xdp_tx_in_flight = 0; rx_ring->xdp.prog = prog;
if (prog)
On Thu, Sep 19, 2024 at 04:41:04PM +0800, Wei Fang wrote:
When running "xdp-bench tx eno0" to test the XDP_TX feature of ENETC on LS1028A, it was found that if the command was re-run multiple times, Rx could not receive the frames, and the result of xdo-bench showed that the rx rate was 0.
root@ls1028ardb:~# ./xdp-bench tx eno0 Hairpinning (XDP_TX) packets on eno0 (ifindex 3; driver fsl_enetc) Summary 2046 rx/s 0 err,drop/s Summary 0 rx/s 0 err,drop/s Summary 0 rx/s 0 err,drop/s Summary 0 rx/s 0 err,drop/s
By observing the Rx PIR and CIR registers, we found that CIR is always equal to 0x7FF and PIR is always 0x7FE, which means that the Rx ring is full and can no longer accommodate other Rx frames. Therefore, it is obvious that the RX BD ring has not been cleaned up.
Further analysis of the code revealed that the Rx BD ring will only be cleaned if the "cleaned_cnt > xdp_tx_in_flight" condition is met. Therefore, some debug logs were added to the driver and the current values of cleaned_cnt and xdp_tx_in_flight were printed when the Rx BD ring was full. The logs are as follows.
[ 178.762419] [XDP TX] >> cleaned_cnt:1728, xdp_tx_in_flight:2140 [ 178.771387] [XDP TX] >> cleaned_cnt:1941, xdp_tx_in_flight:2110 [ 178.776058] [XDP TX] >> cleaned_cnt:1792, xdp_tx_in_flight:2110
From the results, we can see that the maximum value of xdp_tx_in_flight has reached 2140. However, the size of the Rx BD ring is only 2048. This is incredible, so checked the code again and found that the driver did not reset xdp_tx_in_flight when installing or uninstalling bpf program, resulting in xdp_tx_in_flight still retaining the value after the last command was run.
Fixes: c33bfaf91c4c ("net: enetc: set up XDP program under enetc_reconfigure()")
This does not explain why enetc_recycle_xdp_tx_buff(), which decreases xdp_tx_in_flight, does not get called?
In patch 2/3 you wrote:
| Tx BD rings are disabled first in enetc_stop() and then | wait for empty. This operation is not safe while the Tx BD ring | is actively transmitting frames, and will cause the ring to not | be empty and hardware exception. As described in the block guide | of LS1028A NETC, software should only disable an active ring after | all pending ring entries have been consumed (i.e. when PI = CI). | Disabling a transmit ring that is actively processing BDs risks | a HW-SW race hazard whereby a hardware resource becomes assigned | to work on one or more ring entries only to have those entries be | removed due to the ring becoming disabled. So the correct behavior | is that the software stops putting frames on the Tx BD rings (this | is what ENETC_TX_DOWN does), then waits for the Tx BD rings to be | empty, and finally disables the Tx BD rings.
I'm surprised that after fixing that, this change would still be needed, rather than xdp_tx_in_flight naturally dropping down to 0 when stopping NAPI. Why doesn't that happen, and what happens to the pending XDP_TX buffers?
linux-stable-mirror@lists.linaro.org
-
kernel test robot -
Vladimir Oltean -
Wei Fang