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

[Simon Horman <horms@kernel.org>]

On Tue, Apr 30, 2024 at 03:12:58PM +0530, MD Danish Anwar wrote:
> 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.

Thanks, sounds good to me.