netdev - RE: [PATCH net v3 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <SJ1PR11MB6180CA2B18577F8D10E8490DB81DA@SJ1PR11MB6180.namprd11.prod.outlook.com>
Date: Thu, 24 Aug 2023 22:50:34 +0000
From: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "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>, "horms@...nel.org" <horms@...nel.org>,
	"bcreeley@....com" <bcreeley@....com>, Naama Meir
	<naamax.meir@...ux.intel.com>
Subject: RE: [PATCH net v3 2/2] igc: Modify the tx-usecs coalesce setting

Dear Jakub,

Thanks for reviewing 😊

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Thursday, 24 August, 2023 10:19 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
> Cc: davem@...emloft.net; pabeni@...hat.com; edumazet@...gle.com;
> netdev@...r.kernel.org; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@...el.com>; Neftin, Sasha
> <sasha.neftin@...el.com>; horms@...nel.org; bcreeley@....com; Naama
> Meir <naamax.meir@...ux.intel.com>
> Subject: Re: [PATCH net v3 2/2] igc: Modify the tx-usecs coalesce setting
> 
> On Tue, 22 Aug 2023 15:16:20 -0700 Tony Nguyen wrote:
> > root@...DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 netlink error:
> > Invalid argument
> 
> Why was it returning an error previously? It's not clear from just this patch.

In patch 1/2, the returned error was removed. The previous error will prevent the user from entering 
the tx-usecs value; instead, the user can only change the rx-usecs value.

> 
> > -	/* 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;
> > -	else
> > -		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> > +	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;
> > +
> > +		/* convert to rate of irq's per second */
> > +		if (old_tx_itr != ec->tx_coalesce_usecs) {
> > +			adapter->tx_itr_setting =
> > +				igc_ethtool_coalesce_to_itr_setting(ec-
> >tx_coalesce_usecs);
> > +			adapter->rx_itr_setting = adapter->tx_itr_setting;
> > +		} else if (old_rx_itr != ec->rx_coalesce_usecs) {
> > +			adapter->rx_itr_setting =
> > +				igc_ethtool_coalesce_to_itr_setting(ec-
> >rx_coalesce_usecs);
> > +			adapter->tx_itr_setting = adapter->rx_itr_setting;
> > +		}
> 
> I'm not sure about this fix. Systems which try to converge configuration like
> chef will keep issuing:
> 
> ethtool -C enp170s0 tx-usecs 20 rx-usecs 10
> 
> and AFAICT the values will flip back and froth between 10 and 20, and never
> stabilize. Returning an error for unsupported config sounds right to me. This
> function takes extack, you can tell the user what the problem is.

Yeah. In my tests, I missed to set the tx-usecs and rx-usecs together. Thank you for spotting that.
We can add the NL_SET_ERR_MSG_MOD(extack,...) and returning error for unsupported config.
If I recall even if we only set one of the tx or rx usecs, this [.set_coalesce] callback will still provide 
the value of both tx-usecs and rx-usecs. Seems like more checking are needed here.
Do you have any particular thoughts what should be the best case condition here?

Thanks,
Husaini

> --
> pw-bot: cr