Re: [PATCH net-next v2] net: ti: icssg_prueth: Add SW TX / RX Coalescing based on hrtimers

[MD Danish Anwar <danishanwar@ti.com>]

Simon,

On 30/04/24 12:00 am, Simon Horman wrote:
> On Mon, Apr 29, 2024 at 12:45:01PM +0530, MD Danish Anwar wrote:
>> Add SW IRQ coalescing based on hrtimers for RX and TX data path for ICSSG
>> driver, which can be enabled by ethtool commands:
>>
>> - RX coalescing
>>   ethtool -C eth1 rx-usecs 50
>>
>> - TX coalescing can be enabled per TX queue
>>
>>   - by default enables coalesing for TX0
> 
> nit: coalescing
> 
> Please consider running patches through ./checkpatch --codespell
> 
>>   ethtool -C eth1 tx-usecs 50
>>   - configure TX0
>>   ethtool -Q eth0 queue_mask 1 --coalesce tx-usecs 100
>>   - configure TX1
>>   ethtool -Q eth0 queue_mask 2 --coalesce tx-usecs 100
>>   - configure TX0 and TX1
>>   ethtool -Q eth0 queue_mask 3 --coalesce tx-usecs 100 --coalesce
>> tx-usecs 100
>>
>> Minimum value for both rx-usecs and tx-usecs is 20us.
>>
>> Compared to gro_flush_timeout and napi_defer_hard_irqs this patch allows
>> to enable IRQ coalescing for RX path separately.
>>
>> Benchmarking numbers:
>>  ===============================================================
>> | Method                  | Tput_TX | CPU_TX | Tput_RX | CPU_RX |
>> | ==============================================================
>> | Default Driver           943 Mbps    31%      517 Mbps  38%   |
>> | IRQ Coalescing (Patch)   943 Mbps    28%      518 Mbps  25%   |
>>  ===============================================================
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---

[ ... ]
>>  	if (num_tx_packets >= budget)
>>  		return budget;
>>  
>> -	if (napi_complete_done(napi_tx, num_tx_packets))
>> -		enable_irq(tx_chn->irq);
>> +	if (napi_complete_done(napi_tx, num_tx_packets)) {
>> +		if (unlikely(tx_chn->tx_pace_timeout_ns && !tdown)) {
>> +			hrtimer_start(&tx_chn->tx_hrtimer,
>> +				      ns_to_ktime(tx_chn->tx_pace_timeout_ns),
>> +				      HRTIMER_MODE_REL_PINNED);
>> +		} else {
>> +			enable_irq(tx_chn->irq);
>> +		}
> 
> This compiles with gcc-13 and clang-18 W=1
> (although the inner {} are unnecessary).
> 
>> +	}
>>  
>>  	return num_tx_packets;
>>  }
> 
> ...
> 
>> @@ -872,7 +894,13 @@ int emac_napi_rx_poll(struct napi_struct *napi_rx, int budget)
>>  	}
>>  
>>  	if (num_rx < budget && napi_complete_done(napi_rx, num_rx))
>> -		enable_irq(emac->rx_chns.irq[rx_flow]);
>> +		if (unlikely(emac->rx_pace_timeout_ns)) {
>> +			hrtimer_start(&emac->rx_hrtimer,
>> +				      ns_to_ktime(emac->rx_pace_timeout_ns),
>> +				      HRTIMER_MODE_REL_PINNED);
>> +		} else {
>> +			enable_irq(emac->rx_chns.irq[rx_flow]);
>> +		}
> 
> But this does not; I think outer (but not inner) {} are needed.
> 

For both of these if checks, by having {} for outer if I am not seeing
the warnings anymore. The braces don't seem to be neccessary for inner if.

For both of these ifs I'll keep both inner and outer ifs in braces as
this will help readablity as Dan pointed out.

I will post v3 with this change.

> FIIIW, I believe this doesn't show-up in the netdev automated testing
> because this driver isn't built for x86 allmodconfig.
> 
>>  
>>  	return num_rx;
>>  }
> 
> ...
> 

-- 
Thanks and Regards,
Danish