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]

Date: Fri, 25 Aug 2023 03:44:35 +0000
From: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "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



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Friday, 25 August, 2023 8:00 AM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> davem@...emloft.net; pabeni@...hat.com; edumazet@...gle.com;
> netdev@...r.kernel.org; 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 Thu, 24 Aug 2023 22:50:34 +0000 Zulkifli, Muhammad Husaini wrote:
> > > 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.
> 
> I see. Maybe it's better to combine the patches, they are a bit hard to review
> in separation.

IMHO, I would like to separate get and set function in different patch.
Maybe I can add more details in commit message. Is it okay?

> 
> > > 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?
> 
> I was just thinking of something along the lines of:
> 
> if (adapter->flags & IGC_FLAG_QUEUE_PAIRS &&
>     adapter->tx_itr_setting != adapter->rx_itr_setting)
>    ... error ...
> 
> would that work?

Thank you for the suggestion, but it appears that additional checking is required. 
I tested it with the code below, and it appears to work.

		/* convert to rate of irq's per second */
		if ((old_tx_itr != ec->tx_coalesce_usecs) && (old_rx_itr == ec->rx_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) && (old_tx_itr == ec->tx_coalesce_usecs)) {
			adapter->rx_itr_setting =
				igc_ethtool_coalesce_to_itr_setting(ec->rx_coalesce_usecs);
			adapter->tx_itr_setting = adapter->rx_itr_setting;
		} else {
			NL_SET_ERR_MSG_MOD(extack, "Unable to set both TX and RX due to Queue Pairs Flag");
			return -EINVAL;
		}

Thanks,
Husaini