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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2e1db3c654b4e76c7249e90ecf8fa9d64046cbb8.camel@axis.com>
Date: Fri, 25 Aug 2023 13:42:29 +0000
From: Vincent Whitchurch <Vincent.Whitchurch@...s.com>
To: "nbd@....name" <nbd@....name>, "kuba@...nel.org" <kuba@...nel.org>,
	Vincent Whitchurch <Vincent.Whitchurch@...s.com>, "joabreu@...opsys.com"
	<joabreu@...opsys.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"peppe.cavallaro@...com" <peppe.cavallaro@...com>, "alexandre.torgue@...com"
	<alexandre.torgue@...com>
CC: "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, 2023-08-23 at 22:18 +0200, Felix Fietkau wrote:
> Based on tests by OpenWrt users, it seems that this one is causing a 
> significant performance regression caused by wasting lots of CPU cycles 
> re-arming the hrtimer on every single packet. More info:
> https://github.com/openwrt/openwrt/issues/11676#issuecomment-1690492666

It looks like there was an attempt to avoid the re-arming of the timer
in ->xmit() a while ago (pre-dating the hrtimer usage) in commit
4ae0169fd1b3c792b66be58995b7e6b629919ecf ("net: stmmac: Do not keep
rearming the coalesce timer in stmmac_xmit"), but that got reverted
later due to regressions.  The coalescing code has been reworked since
then but the removal of the re-arming was never attempted again.

> My suggestion for fixing this properly would be:
> - keep a separate timestamp for last tx packet
> - do not modify the timer if it's scheduled already
> - in the timer function, check the last tx timestamp and re-arm the 
> timer if necessary.

Would you mind explain the reasons for maintaining a timestamp and
checking it in the expiry function?  Is that to obtain the same effect
as the driver's current behaviour of postponing the expiry of the timer
for each packet?  Is that really desired?  According to the commit
message in 4ae0169fd1b3c792b66be58995b7e6b629919ecf, "Once the timer is
armed it should run after the time expires for the first packet sent and
not the last one."

Since the timer expiry function schedules napi, and the napi poll
function stmmac_tx_clean() re-arms the timer if it sees that there are
pending tx packets, shouldn't an implementation similar to hip04_eth.c
(which doesn't save/check the last tx timestamp) be sufficient?