Received-SPF: pass (google.com: domain of linux-kernel+bounces-164581-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 30 Apr 2024 19:21:42 +0100
From: Simon Horman <horms@kernel.org>
To: MD Danish Anwar <danishanwar@ti.com>
Cc: Dan Carpenter <dan.carpenter@linaro.org>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>, Jan Kiszka <jan.kiszka@siemens.com>,
	Diogo Ivo <diogo.ivo@siemens.com>, Paolo Abeni <pabeni@redhat.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, srk@ti.com,
	Vignesh Raghavendra <vigneshr@ti.com>, r-gunasekaran@ti.com,
	Roger Quadros <rogerq@kernel.org>
Subject: Re: [PATCH net-next v2] net: ti: icssg_prueth: Add SW TX / RX
 Coalescing based on hrtimers
Message-ID: <20240430182142.GC2575892@kernel.org>
References: <20240429071501.547680-1-danishanwar@ti.com>
 <20240429183034.GG516117@kernel.org>
 <5b7cf22a-ca91-4975-bd26-c76a16781ad7@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5b7cf22a-ca91-4975-bd26-c76a16781ad7@ti.com>

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.