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

[PATCH net 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 colease 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 colease setting's value.
Previously, it was restricted to rx-usecs.

The following are changes since commit 5416d7925e6ee72bf1d35cad1957c9a194554da4:
  dt-bindings: net: rockchip-dwmac: fix {tx|rx}-delay defaults/range in schema
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 | 46 +++++++++++++++-----
 1 file changed, 34 insertions(+), 12 deletions(-)

-- 
2.38.1

[PATCH net 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 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 | 33 ++++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 62d925b26f2c..1cf7131a82c5 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -914,6 +914,34 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
 			adapter->flags &= ~IGC_FLAG_DMAC;
 	}
 
+	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;
+
+		if (old_tx_itr != ec->tx_coalesce_usecs) {
+			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;
+
+			adapter->rx_itr_setting = adapter->tx_itr_setting;
+		} else if (old_rx_itr != ec->rx_coalesce_usecs) {
+			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;
+
+			adapter->tx_itr_setting = adapter->rx_itr_setting;
+		}
+		goto program_itr;
+	}
+
 	/* 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;
@@ -921,13 +949,12 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
 		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
 
 	/* 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)
+	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;
 
+program_itr:
 	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 2/2] igc: Modify the tx-usecs coalesce setting

[Simon Horman]

On Fri, Jul 28, 2023 at 10:09:54AM -0700, Tony Nguyen wrote:
> 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 | 33 ++++++++++++++++++--
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 62d925b26f2c..1cf7131a82c5 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -914,6 +914,34 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
>  			adapter->flags &= ~IGC_FLAG_DMAC;
>  	}
>  
> +	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;
> +
> +		if (old_tx_itr != ec->tx_coalesce_usecs) {
> +			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;
> +
> +			adapter->rx_itr_setting = adapter->tx_itr_setting;
> +		} else if (old_rx_itr != ec->rx_coalesce_usecs) {
> +			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;
> +
> +			adapter->tx_itr_setting = adapter->rx_itr_setting;
> +		}
> +		goto program_itr;

This goto seems fairly gratuitous to me.
Couldn't the code be refactored to avoid it,
f.e. by moving ~10 lines below into an else clause?

My main objection here is readability,
I have no objections about correctness.

> +	}
> +
>  	/* 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;
> @@ -921,13 +949,12 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
>  		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
>  
>  	/* 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)
> +	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;
>  
> +program_itr:
>  	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 2/2] igc: Modify the tx-usecs coalesce setting

[Zulkifli, Muhammad Husaini]

Dear Simon,

Thanks for the review.

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Sunday, 30 July, 2023 11:54 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; netdev@vger.kernel.org; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com>; Neftin, Sasha
> <sasha.neftin@intel.com>; Naama Meir <naamax.meir@linux.intel.com>
> Subject: Re: [PATCH net 2/2] igc: Modify the tx-usecs coalesce setting
> 
> On Fri, Jul 28, 2023 at 10:09:54AM -0700, Tony Nguyen wrote:
> > 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 | 33
> > ++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > index 62d925b26f2c..1cf7131a82c5 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > @@ -914,6 +914,34 @@ static int igc_ethtool_set_coalesce(struct net_device
> *netdev,
> >  			adapter->flags &= ~IGC_FLAG_DMAC;
> >  	}
> >
> > +	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;
> > +
> > +		if (old_tx_itr != ec->tx_coalesce_usecs) {
> > +			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;
> > +
> > +			adapter->rx_itr_setting = adapter->tx_itr_setting;
> > +		} else if (old_rx_itr != ec->rx_coalesce_usecs) {
> > +			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;
> > +
> > +			adapter->tx_itr_setting = adapter->rx_itr_setting;
> > +		}
> > +		goto program_itr;
> 
> This goto seems fairly gratuitous to me.
> Couldn't the code be refactored to avoid it, f.e. by moving ~10 lines below into
> an else clause?
> 
> My main objection here is readability,
> I have no objections about correctness.

Good suggestion. I can refactor the code and remove the "goto" statement
as per suggested.

Ex:
if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) {
   ....
} else {
  .....
}

for (i = 0; i < adapter->num_q_vectors; i++) { 
.....

Thanks,
Husaini

> 
> > +	}
> > +
> >  	/* 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; @@ -921,13
> +949,12
> > @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
> >  		adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> >
> >  	/* 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)
> > +	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;
> >
> > +program_itr:
> >  	for (i = 0; i < adapter->num_q_vectors; i++) {
> >  		struct igc_q_vector *q_vector = adapter->q_vector[i];
> >
> > --
> > 2.38.1
> >
> >