Commit title still mentions only XDP_REDIRECT, whereas implementation also touches XDP_TX (and only makes a very minor mention of it).
Wouldn't it be better to have "net: enetc: block concurrent XDP transmissions during ring reconfiguration" for a commit title?
On Wed, Oct 09, 2024 at 05:03:26PM +0800, Wei Fang wrote:
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
detailed
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
(.. driver reconfigures BD rings.) Similarly, XDP_TX verdicts on received frames can also lead to frames being enqueued in the TX rings. Because XDP ignores the state set by the netif_tx_wake_queue() API, we also have to introduce the ENETC_TX_DOWN flag to suppress transmission of XDP frames.
prevent the redirected frames to be attached to Tx BD rings. This is not only used to block XDP_REDIRECT frames, but also to block XDP_TX frames.
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
the driver waits for them to become empty.
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.
It feels like this separate part (refactoring of enetc_start() and enetc_stop() operation ordering) should be its own patch? It is logically different than the introduction and checking of the ENETC_TX_DOWN condition.