Re: [PATCH net] net: stmmac: Use hrtimer for TX coalescing
From: Maxim Mikityanskiy <hidden>
Date: 2023-08-30 18:05:22
On Wed, 30 Aug 2023 at 14:55:37 +0000, Vincent Whitchurch wrote:
Any test results with this patch on the hardware with the performance problems would be appreciated.
TL/DR: it's definitely better than without the patch, but still worse than fully reverting hrtimer [1]. OpenWrt on Netgear R7800, iperf3 test in both directions (LAN->WAN and WAN->LAN), 3 runs for the duration of 1 minute each. OpenWrt options packet_steering (RPS + XPS) and flow_offloading (flowtables) enabled, irqbalance disabled. Numbers in Mbit/s, I had to fit the table into 72 characters, so: U is the original kernel 6.1.46 from OpenWrt, R is reverted hrtimer (patch [1]), P is patched with the new patch from the previous email, ^ is upload (LAN->WAN), v is download (WAN->LAN). | ^ | v | ^ | v | ^ | v | avg ^ | avg v | std ^ | std v - | --- | --- | --- | --- | --- | --- | ----- | ----- | ----- | ----- U | 742 | 709 | 740 | 715 | 556 | 750 | 679 | 725 | 107 | 22 R | 931 | 938 | 935 | 939 | 935 | 939 | 934 | 939 | 2 | 1 P | 845 | 939 | 934 | 939 | 845 | 909 | 875 | 929 | 51 | 17 Full revert allows to get really close to the theoretical maximum goodput (~949 Mbit/s) with minimal deviation. The new patch, however, gives less stable numbers, while sometimes hits the maximum as well. More numbers for the [U]npatched and [R]everted kernels are in the OpenWrt thread [2]. [1]: https://github.com/openwrt/openwrt/files/12422351/revert-stmmac.patch.txt [2]: https://github.com/openwrt/openwrt/issues/11676
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 4727f7be4f86..4b6e5061b5a6 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c@@ -2703,9 +2703,7 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue) /* We still have pending packets, let's call for a new scheduling */ if (tx_q->dirty_tx != tx_q->cur_tx) - hrtimer_start(&tx_q->txtimer, - STMMAC_COAL_TIMER(priv->tx_coal_timer[queue]), - HRTIMER_MODE_REL); + stmmac_tx_timer_arm(priv, queue); __netif_tx_unlock_bh(netdev_get_tx_queue(priv->dev, queue));@@ -2987,6 +2985,20 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue) { struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue]; + /* + * Note that the hrtimer could expire immediately after we check this, + * and the hrtimer and the callers of this function do not share a + * lock. + * + * This should however be safe since the only thing the hrtimer does is + * schedule napi (or ask for it run again if it's already running), and + * stmmac_tx_clean(), called from the napi poll function, also calls + * stmmac_tx_timer_arm() at the end if it sees that there are any TX + * packets which have not yet been cleaned. + */ + if (hrtimer_is_queued(&tx_q->txtimer)) + return; + hrtimer_start(&tx_q->txtimer, STMMAC_COAL_TIMER(priv->tx_coal_timer[queue]), HRTIMER_MODE_REL);