netdev - RE: [PATCH net 2/2] igc: Modify the tx-usecs coalesce setting

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Mon, 31 Jul 2023 03:53:25 +0000
From: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To: Simon Horman <horms@...nel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Neftin, Sasha" <sasha.neftin@...el.com>, "Naama
 Meir" <naamax.meir@...ux.intel.com>
Subject: RE: [PATCH net 2/2] igc: Modify the tx-usecs coalesce setting

Dear Simon,

Thanks for the review.

> -----Original Message-----
> From: Simon Horman <horms@...nel.org>
> Sent: Sunday, 30 July, 2023 11:54 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
> Cc: davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
> edumazet@...gle.com; netdev@...r.kernel.org; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@...el.com>; Neftin, Sasha
> <sasha.neftin@...el.com>; Naama Meir <naamax.meir@...ux.intel.com>
> Subject: Re: [PATCH net 2/2] igc: Modify the tx-usecs coalesce setting
> 
> On Fri, Jul 28, 2023 at 10:09:54AM -0700, Tony Nguyen wrote:
> > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>
> >
> > This patch enables users to modify the tx-usecs parameter.
> > The rx-usecs value will adhere to the same value as tx-usecs if the
> > queue pair setting is enabled.
> >
> > How to test:
> > User can set the coalesce value using ethtool command.
> >
> > Example command:
> > Set: ethtool -C <interface>
> >
> > Previous output:
> >
> > root@...DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 netlink error:
> > Invalid argument
> >
> > New output:
> >
> > root@...DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10
> > rx-usecs: 10
> > rx-frames: n/a
> > rx-usecs-irq: n/a
> > rx-frames-irq: n/a
> >
> > tx-usecs: 10
> > tx-frames: n/a
> > tx-usecs-irq: n/a
> > tx-frames-irq: n/a
> >
> > Fixes: 8c5ad0dae93c ("igc: Add ethtool support")
> > Signed-off-by: Muhammad Husaini Zulkifli
> > <muhammad.husaini.zulkifli@...el.com>
> > Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_ethtool.c | 33
> > ++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > index 62d925b26f2c..1cf7131a82c5 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > @@ -914,6 +914,34 @@ static int igc_ethtool_set_coalesce(struct net_device
> *netdev,
> >  			adapter->flags &= ~IGC_FLAG_DMAC;
> >  	}
> >
> > +	if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) {
> > +		u32 old_tx_itr, old_rx_itr;
> > +
> > +		/* This is to get back the original value before byte shifting */
> > +		old_tx_itr = (adapter->tx_itr_setting <= 3) ?
> > +			      adapter->tx_itr_setting : adapter->tx_itr_setting >>
> 2;
> > +
> > +		old_rx_itr = (adapter->rx_itr_setting <= 3) ?
> > +			      adapter->rx_itr_setting : adapter->rx_itr_setting >>
> 2;
> > +
> > +		if (old_tx_itr != ec->tx_coalesce_usecs) {
> > +			if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs
> <= 3)
> > +				adapter->tx_itr_setting = ec-
> >tx_coalesce_usecs;
> > +			else
> > +				adapter->tx_itr_setting = ec-
> >tx_coalesce_usecs << 2;
> > +
> > +			adapter->rx_itr_setting = adapter->tx_itr_setting;
> > +		} else if (old_rx_itr != ec->rx_coalesce_usecs) {
> > +			if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs
> <= 3)
> > +				adapter->rx_itr_setting = ec-
> >rx_coalesce_usecs;
> > +			else
> > +				adapter->rx_itr_setting = ec-
> >rx_coalesce_usecs << 2;
> > +
> > +			adapter->tx_itr_setting = adapter->rx_itr_setting;
> > +		}
> > +		goto program_itr;
> 
> This goto seems fairly gratuitous to me.
> Couldn't the code be refactored to avoid it, f.e. by moving ~10 lines below into
> an else clause?
> 
> My main objection here is readability,
> I have no objections about correctness.

Good suggestion. I can refactor the code and remove the "goto" statement
as per suggested.

Ex:
if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) {
   ....
} else {
  .....
}

for (i = 0; i < adapter->num_q_vectors; i++) { 
.....

Thanks,
Husaini

> 
> > +	}
> > +
> >  	/* convert to rate of irq's per second */
> >  	if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3)
> >  		adapter->rx_itr_setting = ec->rx_coalesce_usecs; @@ -921,13
> +949,12
> > @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
> >  		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> >
> >  	/* convert to rate of irq's per second */
> > -	if (adapter->flags & IGC_FLAG_QUEUE_PAIRS)
> > -		adapter->tx_itr_setting = adapter->rx_itr_setting;
> > -	else if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3)
> > +	if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3)
> >  		adapter->tx_itr_setting = ec->tx_coalesce_usecs;
> >  	else
> >  		adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2;
> >
> > +program_itr:
> >  	for (i = 0; i < adapter->num_q_vectors; i++) {
> >  		struct igc_q_vector *q_vector = adapter->q_vector[i];
> >
> > --
> > 2.38.1
> >
> >