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

[Felix Fietkau <nbd@nbd.name>]

On 25.08.23 15:42, Vincent Whitchurch wrote:
> 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? 

Exactly. I did something very similar in mac80211 in the past, when 
expensive mod_timer calls were showing up in my perf measurements.
Functionally it should be the same, it just avoids excessive re-sorting 
in timer data structures.

> 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?

To be honest, I didn't look very closely at what the timer does and how 
coalescing works. I don't know if delaying the timer processing with 
every tx is the right choice, or if it should be armed only once. 
However, as you pointed out, the commit that dropped the re-arming was 
reverted because of regressions.

My suggestions are intended to preserve the existing behavior as much as 
possible (in order to avoid regressions), while also achieving the 
benefit of significantly reducing CPU cycles wasted by re-arming the timer.

- Felix