> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@xxxxxxx> > Sent: 2024年10月9日 19:35 > To: Wei Fang <wei.fang@xxxxxxx> > Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; > pabeni@xxxxxxxxxx; Claudiu Manoil <claudiu.manoil@xxxxxxx>; > ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx; hawk@xxxxxxxxxx; > john.fastabend@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > netdev@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx; > imx@xxxxxxxxxxxxxxx; rkannoth@xxxxxxxxxxx; maciej.fijalkowski@xxxxxxxxx; > sbhatta@xxxxxxxxxxx > Subject: Re: [PATCH v3 net 2/3] net: enetc: fix the issues of XDP_REDIRECT > feature > > 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. Okay, I will separate this patch into two patches, one is for ENETC_TX_DOWN, the other is for disabling Tx BDRs after the rings are empty. Thanks.
