Re: [PATCH net] net: stmmac: raise TX completion interrupt at the end of an xmit burst

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Mon, Jul 06, 2026 at 05:32:45AM +0000, Johan Alvarado wrote:
> The TX mitigation logic only sets the Interrupt on Completion bit once
> every tx_coal_frames descriptors (STMMAC_TX_FRAMES = 25), with the
> tx_coal_timer hrtimer (STMMAC_COAL_TX_TIMER = 5000 us) as the only
> fallback. TX skbs are freed exclusively from the TX completion path,
> so any flow that keeps fewer than 25 frames in flight has all of its
> skbs held for up to 5 ms after transmission.
> 
> Paced flows never queue enough frames to reach the frame threshold:
> TCP Small Queues caps the amount of unfreed data at roughly two pacing
> intervals worth, which at moderate pacing rates is only a couple of
> packets. Every small burst then stalls until the coalesce timer fires,
> and throughput collapses to approximately tsq_limit / tx_coal_timer
> regardless of link capacity.
> 
> This is easily reproducible with BBR, which paces its output and thus
> keeps only a few frames in flight at a time. On a YT6801
> (dwmac-motorcomm) equipped Orange Pi 5 Pro, a BBR upload over a ~23 ms
> RTT path is capped at 5.24 Mbit/s, while CUBIC reaches 207 Mbit/s on
> the same path. BBR measures the stalled send rate as the path
> bandwidth and locks its estimate near the floor, so the connection
> never recovers. Lowering the coalesce settings with ethtool -C
> (tx-usecs 100 tx-frames 1) lifts the same transfer to 447 Mbit/s,
> confirming the mechanism.
> 
> Fix this by setting the IC bit on the last descriptor of every xmit
> burst, i.e. whenever netdev_xmit_more() reports that no further frames
> are pending in the current dequeue batch. Frame-based coalescing still
> applies within a burst, bulk traffic keeps batching through qdisc bulk
> dequeue and NAPI polling, and the coalesce timer becomes a pure
> fallback instead of the primary completion mechanism for lightly
> queued flows.
> 
> tx-frames 0 keeps its meaning of timer-based mitigation only.
> 
> Fixes: da2024510031 ("net: stmmac: Tune-up default coalesce settings")
> Signed-off-by: Johan Alvarado <contact@c127.dev>
> ---
> Notes for reviewers (not for the changelog):
> 
> Tested on an Orange Pi 5 Pro (RK3588, Motorcomm YT6801 PCIe GbE via
> dwmac-motorcomm), iperf3 upload to a public server over a ~23 ms RTT
> path, coalesce settings left at their shipped values (tx-usecs 5000,
> tx-frames 25):
> 
>   before, BBR:    5.24 Mbit/s (cwnd pinned, bw estimate ~6 Mbit/s)
>   before, CUBIC:  207 Mbit/s
>   after,  BBR:    447 Mbit/s
> 
> Interrupt overhead stays sane: ~3.3k NIC IRQs/s total at 447 Mbit/s
> (~38 kpps), i.e. roughly 12 packets per interrupt, since qdisc bulk
> dequeue plus NAPI polling still coalesce within bursts.
> 
> The 5000 us STMMAC_COAL_TX_TIMER value postdates the tagged commit
> (it was 1000 us back then); the stall mechanism is the same, only the
> throughput ceiling differs, hence the Fixes tag on the frame-count
> change.
> 
> The XSK/XDP TX paths keep their frame-count-only IC logic: there is
> no skb/TSQ backpressure on those paths, and netdev_xmit_more() is not
> meaningful outside ndo_start_xmit.
> 
> The same completion starvation was reported by Pavel Machek in 2016
> (UDP burst pauses, back then a 40 ms low-res timer):
> https://lore.kernel.org/netdev/20161123105125.GA26394@amd/
> His patch disabling TX coalescing entirely was rejected in favour of
> "a real solution":
> https://lore.kernel.org/netdev/20161205122711.GA30774@amd/

Very messy thread. To reiterate - does tx coalescing do any good in this
driver?

I did some digging and seems there was a rework of coalescing in 2018 and
then some more polishing happened in 2023 (net: stmmac: improve TX timer
arm logic).

> The subsequent hrtimer conversion fixed the timer resolution but kept
> the timer as the only completion mechanism for lightly queued flows;

Wouldn't your change imply that tx coalescing could be dropped altogether?
I do agree that each single batch of tx descs should be signalled with ic
bit at the end.


> this patch adds the missing burst-end interrupt.
> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2a0d7eff88d3..ddf4ac03538d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4626,6 +4626,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  		set_ic = true;
>  	else if (!priv->tx_coal_frames[queue])
>  		set_ic = false;
> +	else if (!netdev_xmit_more())
> +		set_ic = true;
>  	else if (tx_packets > priv->tx_coal_frames[queue])
>  		set_ic = true;
>  	else if ((tx_q->tx_count_frames %
> @@ -4910,6 +4912,8 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  		set_ic = true;
>  	else if (!priv->tx_coal_frames[queue])
>  		set_ic = false;
> +	else if (!netdev_xmit_more())
> +		set_ic = true;
>  	else if (tx_packets > priv->tx_coal_frames[queue])
>  		set_ic = true;
>  	else if ((tx_q->tx_count_frames %
> -- 
> 2.55.0
> 
>