[PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Nick Lowe]

The Intel I211 Ethernet Controller supports 2 Receive Side Scaling (RSS) queues.
It should not be excluded from having this feature enabled.

Via commit c883de9fd787b6f49bf825f3de3601aeb78a7114
E1000_MRQC_ENABLE_RSS_4Q was renamed to E1000_MRQC_ENABLE_RSS_MQ to
indicate that this is a generic bit flag to enable queues and not
a flag that is specific to devices that support 4 queues

The bit flag enables 2, 4 or 8 queues appropriately depending on the part.

Tested with a multicore CPU and frames were then distributed as expected.

This issue appears to have been introduced because of confusion caused
by the prior name.

Signed-off-by: Nick Lowe <nick.lowe@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 03f78fdb0dcd..87ac1d3e25cb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4482,8 +4482,7 @@ static void igb_setup_mrqc(struct igb_adapter *adapter)
 		else
 			mrqc |= E1000_MRQC_ENABLE_VMDQ;
 	} else {
-		if (hw->mac.type != e1000_i211)
-			mrqc |= E1000_MRQC_ENABLE_RSS_MQ;
+		mrqc |= E1000_MRQC_ENABLE_RSS_MQ;
 	}
 	igb_vmm_control(adapter);
 
-- 
2.29.2

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Matt Corallo]

Given this fixes a major (albeit ancient) performance regression, is it not a candidate for backport? It landed on 
Tony's dev-queue branch with a Fixes tag but no stable CC.

Thanks,
Matt

On 12/21/20 5:25 PM, Nick Lowe wrote:
> The Intel I211 Ethernet Controller supports 2 Receive Side Scaling (RSS) queues.
> It should not be excluded from having this feature enabled.
> 
> Via commit c883de9fd787b6f49bf825f3de3601aeb78a7114
> E1000_MRQC_ENABLE_RSS_4Q was renamed to E1000_MRQC_ENABLE_RSS_MQ to
> indicate that this is a generic bit flag to enable queues and not
> a flag that is specific to devices that support 4 queues
> 
> The bit flag enables 2, 4 or 8 queues appropriately depending on the part.
> 
> Tested with a multicore CPU and frames were then distributed as expected.
> 
> This issue appears to have been introduced because of confusion caused
> by the prior name.
> 
> Signed-off-by: Nick Lowe <nick.lowe@gmail.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 03f78fdb0dcd..87ac1d3e25cb 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -4482,8 +4482,7 @@ static void igb_setup_mrqc(struct igb_adapter *adapter)
>   		else
>   			mrqc |= E1000_MRQC_ENABLE_VMDQ;
>   	} else {
> -		if (hw->mac.type != e1000_i211)
> -			mrqc |= E1000_MRQC_ENABLE_RSS_MQ;
> +		mrqc |= E1000_MRQC_ENABLE_RSS_MQ;
>   	}
>   	igb_vmm_control(adapter);
>   
> 

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Jakub Kicinski]

On Sun, 31 Jan 2021 18:17:11 -0500 Matt Corallo wrote:
> Given this fixes a major (albeit ancient) performance regression, is
> it not a candidate for backport? It landed on Tony's dev-queue branch
> with a Fixes tag but no stable CC.

Tony's tree needs to get fed into net, then net into Linus's tree and
then we can do the backport :(

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Nguyen, Anthony L]

On Mon, 2021-02-01 at 08:47 -0800, Jakub Kicinski wrote:
> On Sun, 31 Jan 2021 18:17:11 -0500 Matt Corallo wrote:
> > Given this fixes a major (albeit ancient) performance regression,
> > is
> > it not a candidate for backport? It landed on Tony's dev-queue
> > branch
> > with a Fixes tag but no stable CC.
> 
> Tony's tree needs to get fed into net, then net into Linus's tree and
> then we can do the backport :(

The behavior looks to have always been since support was introduced
with f96a8a0b7854 ("igb: Add Support for new i210/i211 devices."). I
was unable to determine the original reason for excluding it so I was
planning on sending through net-next as added functionality, however, I
will go ahead and send this through net and add it to the current
series that I need to make changes to.

Thanks,
Tony

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Jakub Kicinski]

On Mon, 1 Feb 2021 18:32:50 +0000 Nguyen, Anthony L wrote:
> On Mon, 2021-02-01 at 08:47 -0800, Jakub Kicinski wrote:
> > On Sun, 31 Jan 2021 18:17:11 -0500 Matt Corallo wrote:  
> > > Given this fixes a major (albeit ancient) performance regression,
> > > is
> > > it not a candidate for backport? It landed on Tony's dev-queue
> > > branch
> > > with a Fixes tag but no stable CC.  
> > 
> > Tony's tree needs to get fed into net, then net into Linus's tree and
> > then we can do the backport :(  
> 
> The behavior looks to have always been since support was introduced
> with f96a8a0b7854 ("igb: Add Support for new i210/i211 devices."). I
> was unable to determine the original reason for excluding it so I was
> planning on sending through net-next as added functionality, however, I
> will go ahead and send this through net and add it to the current
> series that I need to make changes to.

Hm, no need for net if it's not really a regression IMO. Not after -rc6.

Matt, would you mind clarifying if this is indeed a regression for i211?

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Nick Lowe]

Hi all,

It is correct that this is not a regression, it instead has never worked.

My suggestion would be to merge it during the 5.11 cycle rather than
5.12 where possible because the patch is non-invasive and should be
lower risk.

There are significant and tangible real-world benefits to RSS
functioning with the i211 so I do not personally think, on balance,
that it is proportionate and worth waiting for 5.12 to conclude here.
Any thoughts?

More importantly though, after this does release in a stable kernel
(regardless of whether that is 5.11 or 5.12) and this change has had
some time to soak in the field, I do suggest consideration to then
backport this to the 5.4 and 5.10 LTS kernels so that RSS starts to
function there. 5.4 and 5.10 are the release branches that will, of
course, get far more use for the next couple of years.

Best,

Nick

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Matt Corallo]

On 2/1/21 2:45 PM, Jakub Kicinski wrote:
> Matt, would you mind clarifying if this is indeed a regression for i211?
> 

Admittedly, I didn't go all the way back to test that it is, indeed, a regression. The Fixes commit that it was tagged 
with on Tony's tree was something more recent than initial igb landing (its a refactor of RSS) and there are numerous 
posts online indicating common I211 hardware (eg PCEngines APU2) support RSS and properly load multiple cores, so I 
naively assumed that it is a regression of some form.

Did you test kernels odler than c883de9fd787, Nick?

Given that, and the non-invasive nature of the patch, I figured it was worth trying to land on LTS trees and going ahead 
with it for 5.11, but I don't feel strongly on how it ends up on LTS, it just seems a waste to not land it there.

Matt

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Nick Lowe]

I personally tested with mainline and 5.10, but not 5.4

Best,

Nick

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Jakub Kicinski]

On Mon, 1 Feb 2021 20:23:51 +0000 Nick Lowe wrote:
> I personally tested with mainline and 5.10, but not 5.4

My preference would be to merge into net-next, and then do the
backport after 5.11 is released.

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Nick Lowe]

Hi all,

Now that the 5.12 kernel has released, please may we consider
backporting commit 6e6026f2dd2005844fb35c3911e8083c09952c6c to both
the 5.4 and 5.10 LTS kernels so that RSS starts to function with the
i211?

Best,

Nick

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Jakub Kicinski]

On Mon, 3 May 2021 13:32:24 +0100 Nick Lowe wrote:
> Hi all,
> 
> Now that the 5.12 kernel has released, please may we consider
> backporting commit 6e6026f2dd2005844fb35c3911e8083c09952c6c to both
> the 5.4 and 5.10 LTS kernels so that RSS starts to function with the
> i211?

No objections here. Please submit the backport request to stable@.
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-2

Stable inclusion request - igb: Enable RSS for Intel I211 Ethernet Controller

[Nick Lowe]

Hi,

Please may we request that commit 6e6026f2dd20 ("igb: Enable RSS for
Intel I211 Ethernet Controller") be backported to the 5.4 and 5.10 LTS
kernels?

The Intel i211 Ethernet Controller supports 2 Receive Side Scaling
(RSS) queues, the patch corrects the issue that the i211 should not be
excluded from having this feature enabled.

Best regards,

Nick

---------- Forwarded message ---------
From: Jakub Kicinski <kuba@kernel.org>
Date: Mon, 3 May 2021 at 19:30
Subject: Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller
To: Nick Lowe <nick.lowe@gmail.com>
Cc: Matt Corallo <linux-wired-list@bluematt.me>, Nguyen, Anthony L
<anthony.l.nguyen@intel.com>, netdev@vger.kernel.org
<netdev@vger.kernel.org>, davem@davemloft.net <davem@davemloft.net>,
Brandeburg, Jesse <jesse.brandeburg@intel.com>,
intel-wired-lan@lists.osuosl.org <intel-wired-lan@lists.osuosl.org>


On Mon, 3 May 2021 13:32:24 +0100 Nick Lowe wrote:
> Hi all,
>
> Now that the 5.12 kernel has released, please may we consider
> backporting commit 6e6026f2dd2005844fb35c3911e8083c09952c6c to both
> the 5.4 and 5.10 LTS kernels so that RSS starts to function with the
> i211?

No objections here. Please submit the backport request to stable@.
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-2

Re: Stable inclusion request - igb: Enable RSS for Intel I211 Ethernet Controller

[Greg KH]

On Mon, May 03, 2021 at 08:53:48PM +0100, Nick Lowe wrote:
> Hi,
> 
> Please may we request that commit 6e6026f2dd20 ("igb: Enable RSS for
> Intel I211 Ethernet Controller") be backported to the 5.4 and 5.10 LTS
> kernels?
>

Also added to 5.11 as it's still alive for another week or so :)

thanks,

greg k-h

Re: [PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Jakub Kicinski]

On Mon, 21 Dec 2020 19:15:49 +0000 Nick Lowe wrote:
> The Intel I211 Ethernet Controller supports 2 Receive Side Scaling (RSS) queues.
> It should not be excluded from having this feature enabled.
> 
> Via commit c883de9fd787b6f49bf825f3de3601aeb78a7114
> E1000_MRQC_ENABLE_RSS_4Q was renamed to E1000_MRQC_ENABLE_RSS_MQ to
> indicate that this is a generic bit flag to enable queues and not
> a flag that is specific to devices that support 4 queues
> 
> The bit flag enables 2, 4 or 8 queues appropriately depending on the part.
> 
> Tested with a multicore CPU and frames were then distributed as expected.
> 
> This issue appears to have been introduced because of confusion caused
> by the prior name.
> 
> Signed-off-by: Nick Lowe <nick.lowe@gmail.com>

Please repost CCing the maintainers.

[PATCH net] igb: Enable RSS for Intel I211 Ethernet Controller

[Nick Lowe]

The Intel I211 Ethernet Controller supports 2 Receive Side Scaling (RSS) queues.
It should not be excluded from having this feature enabled.

Via commit c883de9fd787b6f49bf825f3de3601aeb78a7114
E1000_MRQC_ENABLE_RSS_4Q was renamed to E1000_MRQC_ENABLE_RSS_MQ to
indicate that this is a generic bit flag to enable queues and not
a flag that is specific to devices that support 4 queues

The bit flag enables 2, 4 or 8 queues appropriately depending on the part.

Tested with a multicore CPU and frames were then distributed as expected.

This issue appears to have been introduced because of confusion caused
by the prior name.

Signed-off-by: Nick Lowe <nick.lowe@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 03f78fdb0dcd..87ac1d3e25cb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4482,8 +4482,7 @@ static void igb_setup_mrqc(struct igb_adapter *adapter)
 		else
 			mrqc |= E1000_MRQC_ENABLE_VMDQ;
 	} else {
-		if (hw->mac.type != e1000_i211)
-			mrqc |= E1000_MRQC_ENABLE_RSS_MQ;
+		mrqc |= E1000_MRQC_ENABLE_RSS_MQ;
 	}
 	igb_vmm_control(adapter);
 
-- 
2.29.2