[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZO-E2_A-UrC9127S@mail.gmail.com>
Date: Wed, 30 Aug 2023 21:05:15 +0300
From: Maxim Mikityanskiy <maxtram95@...il.com>
To: Vincent Whitchurch <Vincent.Whitchurch@...s.com>
Cc: "alexandre.torgue@...com" <alexandre.torgue@...com>,
"nbd@....name" <nbd@....name>, "kuba@...nel.org" <kuba@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"joabreu@...opsys.com" <joabreu@...opsys.com>,
"peppe.cavallaro@...com" <peppe.cavallaro@...com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
kernel <kernel@...s.com>
Subject: Re: [PATCH net] net: stmmac: Use hrtimer for TX coalescing
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);
>
Powered by blists - more mailing lists