Re: [PATCH net] net: stmmac: Use hrtimer for TX coalescing

[Maxim Mikityanskiy <maxtram95@gmail.com>]

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

> 
> 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);
>