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: Thu, 24 Aug 2023 17:00:22 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
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

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.

> > 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?