| Message ID | 20201221222502.1706-1-nick.lowe@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] igb: Enable RSS for Intel I211 Ethernet Controller | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 17 this patch: 17 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 17 this patch: 17 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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); > >
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 :(
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
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?
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
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
I personally tested with mainline and 5.10, but not 5.4 Best, Nick
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.
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
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
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
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
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);
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(-)