[PATCH net v3 0/2][pull request] igc: Enhance the tx-usecs coalesce setting implementation

[PATCH net v3 0/2][pull request] igc: Enhance the tx-usecs coalesce setting implementation

[Tony Nguyen]

Muhammad Husaini Zulkifli says:

The current tx-usecs coalesce setting implementation in the driver code is
improved by this patch series. The implementation of the current driver
code may have previously been a copy of the legacy code i210.

Patch 1:
Allow the user to see the tx-usecs coalesce setting's current value when
using the ethtool command. The previous value was 0.

Patch 2:
Give the user the ability to modify the tx-usecs coalesce setting's value.
Previously, it was restricted to rx-usecs.
---
v3:
- Implement the helper function, as recommended by Brett Creeley.
- Fix typo in cover letter.

v2: https://lore.kernel.org/netdev/20230801172742.3625719-1-anthony.l.nguyen@intel.com/
- Refactor the code, as Simon suggested, to make it more readable.

v1: https://lore.kernel.org/netdev/20230728170954.2445592-1-anthony.l.nguyen@intel.com/

The following are changes since commit 99b415fe8986803ba0eaf6b8897b16edc8fe7ec2:
  tg3: Use slab_build_skb() when needed
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 1GbE

Muhammad Husaini Zulkifli (2):
  igc: Expose tx-usecs coalesce setting to user
  igc: Modify the tx-usecs coalesce setting

 drivers/net/ethernet/intel/igc/igc_ethtool.c | 58 +++++++++++++-------
 1 file changed, 37 insertions(+), 21 deletions(-)

-- 
2.38.1

[PATCH net v3 1/2] igc: Expose tx-usecs coalesce setting to user

[Tony Nguyen]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

When users attempt to obtain the coalesce setting using the
ethtool command, current code always returns 0 for tx-usecs.
This is because I225/6 always uses a queue pair setting, hence
tx_coalesce_usecs does not return a value during the
igc_ethtool_get_coalesce() callback process. The pair queue
condition checking in igc_ethtool_get_coalesce() is removed by
this patch so that the user gets information of the value of tx-usecs.

Even if i225/6 is using queue pair setting, there is no harm in
notifying the user of the tx-usecs. The implementation of the current
code may have previously been a copy of the legacy code i210.

How to test:
User can get the coalesce value using ethtool command.

Example command:
Get: ethtool -c <interface>

Previous output:

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

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

New output:

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

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

Fixes: 8c5ad0dae93c ("igc: Add ethtool support")
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 93bce729be76..62d925b26f2c 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -880,12 +880,10 @@ static int igc_ethtool_get_coalesce(struct net_device *netdev,
 	else
 		ec->rx_coalesce_usecs = adapter->rx_itr_setting >> 2;
 
-	if (!(adapter->flags & IGC_FLAG_QUEUE_PAIRS)) {
-		if (adapter->tx_itr_setting <= 3)
-			ec->tx_coalesce_usecs = adapter->tx_itr_setting;
-		else
-			ec->tx_coalesce_usecs = adapter->tx_itr_setting >> 2;
-	}
+	if (adapter->tx_itr_setting <= 3)
+		ec->tx_coalesce_usecs = adapter->tx_itr_setting;
+	else
+		ec->tx_coalesce_usecs = adapter->tx_itr_setting >> 2;
 
 	return 0;
 }
@@ -910,9 +908,6 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
 	    ec->tx_coalesce_usecs == 2)
 		return -EINVAL;
 
-	if ((adapter->flags & IGC_FLAG_QUEUE_PAIRS) && ec->tx_coalesce_usecs)
-		return -EINVAL;
-
 	/* If ITR is disabled, disable DMAC */
 	if (ec->rx_coalesce_usecs == 0) {
 		if (adapter->flags & IGC_FLAG_DMAC)
-- 
2.38.1

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

[Tony Nguyen]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

This patch enables users to modify the tx-usecs parameter.
The rx-usecs value will adhere to the same value as tx-usecs
if the queue pair setting is enabled.

How to test:
User can set the coalesce value using ethtool command.

Example command:
Set: ethtool -C <interface>

Previous output:

root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10
netlink error: Invalid argument

New output:

root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10
rx-usecs: 10
rx-frames: n/a
rx-usecs-irq: n/a
rx-frames-irq: n/a

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

Fixes: 8c5ad0dae93c ("igc: Add ethtool support")
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 45 ++++++++++++++------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 62d925b26f2c..40ec6ebc0843 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -888,6 +888,11 @@ static int igc_ethtool_get_coalesce(struct net_device *netdev,
 	return 0;
 }
 
+static int igc_ethtool_coalesce_to_itr_setting(u32 coalesce_usecs)
+{
+	return coalesce_usecs <= 3 ? coalesce_usecs : coalesce_usecs << 2;
+}
+
 static int igc_ethtool_set_coalesce(struct net_device *netdev,
 				    struct ethtool_coalesce *ec,
 				    struct kernel_ethtool_coalesce *kernel_coal,
@@ -914,19 +919,35 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
 			adapter->flags &= ~IGC_FLAG_DMAC;
 	}
 
-	/* convert to rate of irq's per second */
-	if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3)
-		adapter->rx_itr_setting = ec->rx_coalesce_usecs;
-	else
-		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
+	if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) {
+		u32 old_tx_itr, old_rx_itr;
+
+		/* This is to get back the original value before byte shifting */
+		old_tx_itr = (adapter->tx_itr_setting <= 3) ?
+			      adapter->tx_itr_setting : adapter->tx_itr_setting >> 2;
+
+		old_rx_itr = (adapter->rx_itr_setting <= 3) ?
+			      adapter->rx_itr_setting : adapter->rx_itr_setting >> 2;
+
+		/* convert to rate of irq's per second */
+		if (old_tx_itr != ec->tx_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) {
+			adapter->rx_itr_setting =
+				igc_ethtool_coalesce_to_itr_setting(ec->rx_coalesce_usecs);
+			adapter->tx_itr_setting = adapter->rx_itr_setting;
+		}
+	} else {
+		/* convert to rate of irq's per second */
+		adapter->rx_itr_setting =
+			igc_ethtool_coalesce_to_itr_setting(ec->rx_coalesce_usecs);
 
-	/* convert to rate of irq's per second */
-	if (adapter->flags & IGC_FLAG_QUEUE_PAIRS)
-		adapter->tx_itr_setting = adapter->rx_itr_setting;
-	else if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3)
-		adapter->tx_itr_setting = ec->tx_coalesce_usecs;
-	else
-		adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2;
+		/* convert to rate of irq's per second */
+		adapter->tx_itr_setting =
+			igc_ethtool_coalesce_to_itr_setting(ec->tx_coalesce_usecs);
+	}
 
 	for (i = 0; i < adapter->num_q_vectors; i++) {
 		struct igc_q_vector *q_vector = adapter->q_vector[i];
-- 
2.38.1

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

[Jakub Kicinski]

On Tue, 22 Aug 2023 15:16:20 -0700 Tony Nguyen wrote:
> root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10
> netlink error: Invalid argument

Why was it returning an error previously? It's not clear from just 
this patch.

> -	/* convert to rate of irq's per second */
> -	if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3)
> -		adapter->rx_itr_setting = ec->rx_coalesce_usecs;
> -	else
> -		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> +	if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) {
> +		u32 old_tx_itr, old_rx_itr;
> +
> +		/* This is to get back the original value before byte shifting */
> +		old_tx_itr = (adapter->tx_itr_setting <= 3) ?
> +			      adapter->tx_itr_setting : adapter->tx_itr_setting >> 2;
> +
> +		old_rx_itr = (adapter->rx_itr_setting <= 3) ?
> +			      adapter->rx_itr_setting : adapter->rx_itr_setting >> 2;
> +
> +		/* convert to rate of irq's per second */
> +		if (old_tx_itr != ec->tx_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) {
> +			adapter->rx_itr_setting =
> +				igc_ethtool_coalesce_to_itr_setting(ec->rx_coalesce_usecs);
> +			adapter->tx_itr_setting = adapter->rx_itr_setting;
> +		}

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.
-- 
pw-bot: cr

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

[Zulkifli, Muhammad Husaini]

Dear Jakub,

Thanks for reviewing 😊

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, 24 August, 2023 10:19 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com>; Neftin, Sasha
> <sasha.neftin@intel.com>; horms@kernel.org; bcreeley@amd.com; Naama
> Meir <naamax.meir@linux.intel.com>
> Subject: Re: [PATCH net v3 2/2] igc: Modify the tx-usecs coalesce setting
> 
> On Tue, 22 Aug 2023 15:16:20 -0700 Tony Nguyen wrote:
> > root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 netlink error:
> > Invalid argument
> 
> 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.

> 
> > -	/* convert to rate of irq's per second */
> > -	if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3)
> > -		adapter->rx_itr_setting = ec->rx_coalesce_usecs;
> > -	else
> > -		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> > +	if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) {
> > +		u32 old_tx_itr, old_rx_itr;
> > +
> > +		/* This is to get back the original value before byte shifting */
> > +		old_tx_itr = (adapter->tx_itr_setting <= 3) ?
> > +			      adapter->tx_itr_setting : adapter->tx_itr_setting
> >> 2;
> > +
> > +		old_rx_itr = (adapter->rx_itr_setting <= 3) ?
> > +			      adapter->rx_itr_setting : adapter->rx_itr_setting
> >> 2;
> > +
> > +		/* convert to rate of irq's per second */
> > +		if (old_tx_itr != ec->tx_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) {
> > +			adapter->rx_itr_setting =
> > +				igc_ethtool_coalesce_to_itr_setting(ec-
> >rx_coalesce_usecs);
> > +			adapter->tx_itr_setting = adapter->rx_itr_setting;
> > +		}
> 
> 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?

Thanks,
Husaini

> --
> pw-bot: cr

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

[Jakub Kicinski]

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?

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

[Zulkifli, Muhammad Husaini]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, 25 August, 2023 8:00 AM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org; Neftin, Sasha <sasha.neftin@intel.com>;
> horms@kernel.org; bcreeley@amd.com; Naama Meir
> <naamax.meir@linux.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.

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?

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

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

Thanks,
Husaini

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

[Jakub Kicinski]

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.

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

[Zulkifli, Muhammad Husaini]

> 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@P12DYHUSAINI:~# ethtool -C enp1s0 rx-usecs 14 tx-usecs 13
root@P12DYHUSAINI:~#

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

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

[Jakub Kicinski]

On Mon, 4 Sep 2023 00:59:40 +0000 Zulkifli, Muhammad Husaini wrote:
> 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?

It's unfortunate, but strictly better than the alternative, AFACT.
In the ioctl uAPI we can't differentiate between params which were
echoed back to us vs those user set via CLI to what they already were.

Maybe we should extend the uAPI and add a "queue pair" IRQ moderation?

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

[Zulkifli, Muhammad Husaini]

> On Mon, 4 Sep 2023 00:59:40 +0000 Zulkifli, Muhammad Husaini wrote:
> > 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?
> 
> It's unfortunate, but strictly better than the alternative, AFACT.

Agree.

> In the ioctl uAPI we can't differentiate between params which were echoed back
> to us vs those user set via CLI to what they already were.
> 
> Maybe we should extend the uAPI and add a "queue pair" IRQ moderation?

Good advice. BTW, if queue pair setting is enabled in the driver, could we change the existing ".supported_coalesce_params" for driver specific?

From:
ETHTOOL_COALESCE_USECS which support (ETHTOOL_COALESCE_RX_USECS | ETHTOOL_COALESCE_TX_USECS)

To (new define):
ETHTOOL_QUEUE_PAIR_COALESCE_USECS (ETHTOOL_COALESCE_RX_USECS)

With this, I believe user cannot set tx-usecs and will return error of unsupported parameters.

Thanks,
Husaini

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

[Jakub Kicinski]

On Wed, 6 Sep 2023 02:52:30 +0000 Zulkifli, Muhammad Husaini wrote:
> > In the ioctl uAPI we can't differentiate between params which were echoed back
> > to us vs those user set via CLI to what they already were.
> > 
> > Maybe we should extend the uAPI and add a "queue pair" IRQ moderation?  
> 
> Good advice. BTW, if queue pair setting is enabled in the driver, could we change the existing ".supported_coalesce_params" for driver specific?
> 
> From:
> ETHTOOL_COALESCE_USECS which support (ETHTOOL_COALESCE_RX_USECS | ETHTOOL_COALESCE_TX_USECS)
> 
> To (new define):
> ETHTOOL_QUEUE_PAIR_COALESCE_USECS (ETHTOOL_COALESCE_RX_USECS)
> 
> With this, I believe user cannot set tx-usecs and will return error of unsupported parameters.

Do you mean change the .supported_coalesce_params at runtime?
I think so far we were expecting drivers to set flags for all types
they may support and then reject the settings incompatible with current
operation mode at the driver level.

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

[Zulkifli, Muhammad Husaini]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, 6 September, 2023 10:47 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org; Neftin,
> Sasha <sasha.neftin@intel.com>; horms@kernel.org; bcreeley@amd.com;
> Naama Meir <naamax.meir@linux.intel.com>
> Subject: Re: [PATCH net v3 2/2] igc: Modify the tx-usecs coalesce setting
> 
> On Wed, 6 Sep 2023 02:52:30 +0000 Zulkifli, Muhammad Husaini wrote:
> > > In the ioctl uAPI we can't differentiate between params which were
> > > echoed back to us vs those user set via CLI to what they already were.
> > >
> > > Maybe we should extend the uAPI and add a "queue pair" IRQ moderation?
> >
> > Good advice. BTW, if queue pair setting is enabled in the driver, could we
> change the existing ".supported_coalesce_params" for driver specific?
> >
> > From:
> > ETHTOOL_COALESCE_USECS which support (ETHTOOL_COALESCE_RX_USECS
> |
> > ETHTOOL_COALESCE_TX_USECS)
> >
> > To (new define):
> > ETHTOOL_QUEUE_PAIR_COALESCE_USECS (ETHTOOL_COALESCE_RX_USECS)
> >
> > With this, I believe user cannot set tx-usecs and will return error of
> unsupported parameters.
> 
> Do you mean change the .supported_coalesce_params at runtime?
> I think so far we were expecting drivers to set flags for all types they may
> support and then reject the settings incompatible with current operation mode
> at the driver level.

It doesn't seem like a runtime. 
The queue pair parameters that I stated previously can be set as soon as we want to register the ethtool operations. 
Currently it was set to ETHTOOL_COALESCE_USECS which supporting both tx-usecs and rx-usecs.
Thus, by restricting to only supporting rx-usecs with the new define of ETHTOOL_QUEUE_PAIR_COALESCE_USECS
would be useful in this situation IMHO.

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L1980
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L1939