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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 4 Sep 2023 00:59:40 +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

> 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 am okay to squash it together. More explanation below.

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

The original code does this action. Only changed "rx-usecs" is allowed.
However, my intention is to inform the user of the tx-usecs value when they execute the "ethtool -c interface>" command.
Previously, when we ran the previous command, the value of tx-usecs was always 0.
My plan is to display the same tx-usecs value as rx-usecs, or vice versa.

And.. it need some changes on original code below:

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L883 
Removing this will allow tx-usecs to have the value but it will cause some bug for changes in L913 below. 

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L913
To set the rx-usecs/tx-usecs, this line must be removed/modify; otherwise tx-usecs will always have a value whenever the callback is in, which is the reason.
Previously without removing this, we not able to set rx-usecs.

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

Yeah, we can also just modify at the line https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L913 similar to what you proposed:

	if ((adapter->flags & IGC_FLAG_QUEUE_PAIRS) &&
		ec->tx_coalesce_usecs != igc_ethtool_get_previous_tx_coalesce(adapter)) {
		NL_SET_ERR_MSG_MOD(extack, "Queue Pair mode enabled, both Rx and Tx coalescing controlled by rx-usecs");
		return -EINVAL;
	}

However, if the user enters the same value for the tx-usecs and a different value for the rx-usecs, the command will succeed. . Are you ok with it?
I am not sure who is going to do like this but yeah....... Unless we blocking 2 inputs argument

rx-usecs: 13
rx-frames: n/a
rx-usecs-irq: n/a
rx-frames-irq: n/a

tx-usecs: 13
tx-frames: n/a
tx-usecs-irq: n/a
tx-frames-irq: n/a

rx-usecs-low: n/a
rx-frame-low: n/a
tx-usecs-low: n/a
tx-frame-low: n/a

rx-usecs-high: n/a
rx-frame-high: n/a
tx-usecs-high: n/a
tx-frame-high: n/a

CQE mode RX: n/a  TX: n/a

root@...DYHUSAINI:~# ethtool -C enp1s0 rx-usecs 14 tx-usecs 13
root@...DYHUSAINI:~#

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

Yeah let's make it simple 😊