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: <20230825173429.2a2d0d9f@kernel.org>
Date: Fri, 25 Aug 2023 17:34:29 -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 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.