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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240430182142.GC2575892@kernel.org>
Date: Tue, 30 Apr 2024 19:21:42 +0100
From: Simon Horman <horms@...nel.org>
To: MD Danish Anwar <danishanwar@...com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Andrew Lunn <andrew@...n.ch>, Jan Kiszka <jan.kiszka@...mens.com>,
	Diogo Ivo <diogo.ivo@...mens.com>, Paolo Abeni <pabeni@...hat.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, srk@...com,
	Vignesh Raghavendra <vigneshr@...com>, r-gunasekaran@...com,
	Roger Quadros <rogerq@...nel.org>
Subject: Re: [PATCH net-next v2] net: ti: icssg_prueth: Add SW TX / RX
 Coalescing based on hrtimers

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@...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.