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

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 25 Aug 2023 03:44:35 +0000 Zulkifli, Muhammad Husaini wrote:
> > 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?

That's exactly what confused me. You made it sound like first patch is
only about GET but it actually also changes SET.

> > 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;
> 		}

What if user space does:

  cmd = "ethtool -C eth0 "
  if rx_mismatch:
    cmd += "rx-usecs " + rxu + " "
  if tx_mismatch:
    cmd += "tx-usecs " + txu + " "
  system(cmd)

Why do you think that the auto-update of the other value matters 
so much? With a clear warning user should be able to figure
out that they need to set the values identically.

If you want to auto-update maybe only allow rx-usecs changes?

Basically:

  if (old_tx != ec->tx_coalesce_usecs) {
    NL_SET_ERR_MSG_MOD(extack, "Queue Pair mode enabled, both Rx and Tx coalescing controlled by rx-usecs");
   return -EINVAL;
  }
  rx_itr = tx_itr = logic(ec->tx_coalesce_usecs);


I hate these auto-magic changes, because I had to email support /
vendors in the past asking them "what does the device _actually_
do" / "what is the device capable of" due to the driver doing magic.
The API is fairly clear. If user wants rx and tx to be different,
and the device does not support that -- error.