Re: [PATCH net v3 2/2] igc: Modify the tx-usecs coalesce setting

[Jakub Kicinski <kuba@kernel.org>]

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?