[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Alexander Duyck]

It appears that when the i211 was introduced it was never actually enabling
RSS.  This change makes it so that we enable RSS by setting the appropriate
bit in the MRQC register.

Reported-by: Tal Abudi <talabudi@gmail.com>
Signed-off-by: Alexander Duyck <aduyck@mirantis.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 7afde455326d..c803da0aa883 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3464,8 +3464,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_4Q;
+		mrqc |= E1000_MRQC_ENABLE_RSS_4Q;
 	}
 	igb_vmm_control(adapter);
 

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Fujinaka, Todd]

I've spent too much time looking into this. I think this was added when someone looked for the register value and it was the same as E1000_MRQC_ENABLE_RSS_8Q and disabled i211 because it doesn't have 8 queues.

I've been asked if you could spin this to add a comment that the two values are the same (E1000_MRQC_ENABLE_RSS_8Q and E1000_MRQC_ENABLE_RSS_4Q).

Thanks.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka at intel.com
(503) 712-4565


-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Alexander Duyck
Sent: Wednesday, December 16, 2015 11:31 AM
To: intel-wired-lan@lists.osuosl.org; alexander.duyck at gmail.com
Cc: Tal Abudi
Subject: [Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

It appears that when the i211 was introduced it was never actually enabling RSS.  This change makes it so that we enable RSS by setting the appropriate bit in the MRQC register.

Reported-by: Tal Abudi <talabudi@gmail.com>
Signed-off-by: Alexander Duyck <aduyck@mirantis.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 7afde455326d..c803da0aa883 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3464,8 +3464,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_4Q;
+		mrqc |= E1000_MRQC_ENABLE_RSS_4Q;
 	}
 	igb_vmm_control(adapter);
 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Alex Duyck]

On Thu, Dec 17, 2015 at 10:37 AM, Fujinaka, Todd
<todd.fujinaka@intel.com> wrote:
> I've spent too much time looking into this. I think this was added when someone looked for the register value and it was the same as E1000_MRQC_ENABLE_RSS_8Q and disabled i211 because it doesn't have 8 queues.
>
> I've been asked if you could spin this to add a comment that the two values are the same (E1000_MRQC_ENABLE_RSS_8Q and E1000_MRQC_ENABLE_RSS_4Q).

That would be even more confusing since the value that is being used
is RSS_4Q so I am not sure where the 8Q comes into play.  I guess at
some point it was renamed?  Feel free to modify the code however you
want.  I was just going through and trying to figure out the issue
that had been reported and I noticed it wasn't getting enabled.  It's
fine with me if you drop me as author if you guys rewrite the patch
and add a comment.  You could probably just add a Suggested-by credit
to the patch you generate.

- Alex

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Fujinaka, Todd]

As I was composing this email, I noticed the same thing. I'll double-check to make sure the comment is actually necessary.

Thanks Alex!

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka at intel.com
(503) 712-4565


-----Original Message-----
From: Alex Duyck [mailto:aduyck at mirantis.com] 
Sent: Thursday, December 17, 2015 10:43 AM
To: Fujinaka, Todd
Cc: intel-wired-lan at lists.osuosl.org; alexander.duyck at gmail.com; Tal Abudi
Subject: Re: [Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

On Thu, Dec 17, 2015 at 10:37 AM, Fujinaka, Todd <todd.fujinaka@intel.com> wrote:
> I've spent too much time looking into this. I think this was added when someone looked for the register value and it was the same as E1000_MRQC_ENABLE_RSS_8Q and disabled i211 because it doesn't have 8 queues.
>
> I've been asked if you could spin this to add a comment that the two values are the same (E1000_MRQC_ENABLE_RSS_8Q and E1000_MRQC_ENABLE_RSS_4Q).

That would be even more confusing since the value that is being used is RSS_4Q so I am not sure where the 8Q comes into play.  I guess at some point it was renamed?  Feel free to modify the code however you want.  I was just going through and trying to figure out the issue that had been reported and I noticed it wasn't getting enabled.  It's fine with me if you drop me as author if you guys rewrite the patch and add a comment.  You could probably just add a Suggested-by credit to the patch you generate.

- Alex

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Tal Abudi]

The I211 supports only 2 queues according to the datasheet.
Is there a real difference between E1000_MRQC_ENABLE_RSS_4Q and
E1000_MRQC_ENABLE_RSS_8Q
?
The number of the rss is set inside the adapter and the netdevice's dev.
And we can see in igb_init_queue_configuration() that max_rss_queues =
IGB_MAX_RX_QUEUES_I211 (2).

I tested 5.3.2 with this patch on a 2 core system and packets distributed
evenly among queues.

This is most defective a bug, right ?

On Thu, Dec 17, 2015 at 8:52 PM, Fujinaka, Todd <todd.fujinaka@intel.com>
wrote:

> As I was composing this email, I noticed the same thing. I'll double-check
> to make sure the comment is actually necessary.
>
> Thanks Alex!
>
> Todd Fujinaka
> Software Application Engineer
> Networking Division (ND)
> Intel Corporation
> todd.fujinaka at intel.com
> (503) 712-4565
>
>
> -----Original Message-----
> From: Alex Duyck [mailto:aduyck at mirantis.com]
> Sent: Thursday, December 17, 2015 10:43 AM
> To: Fujinaka, Todd
> Cc: intel-wired-lan at lists.osuosl.org; alexander.duyck at gmail.com; Tal Abudi
> Subject: Re: [Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter
>
> On Thu, Dec 17, 2015 at 10:37 AM, Fujinaka, Todd <todd.fujinaka@intel.com>
> wrote:
> > I've spent too much time looking into this. I think this was added when
> someone looked for the register value and it was the same as
> E1000_MRQC_ENABLE_RSS_8Q and disabled i211 because it doesn't have 8 queues.
> >
> > I've been asked if you could spin this to add a comment that the two
> values are the same (E1000_MRQC_ENABLE_RSS_8Q and E1000_MRQC_ENABLE_RSS_4Q).
>
> That would be even more confusing since the value that is being used is
> RSS_4Q so I am not sure where the 8Q comes into play.  I guess at some
> point it was renamed?  Feel free to modify the code however you want.  I
> was just going through and trying to figure out the issue that had been
> reported and I noticed it wasn't getting enabled.  It's fine with me if you
> drop me as author if you guys rewrite the patch and add a comment.  You
> could probably just add a Suggested-by credit to the patch you generate.
>
> - Alex
>



-- 
Best regards,
Tal Abudi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20151218/7e6c0481/attachment.html>

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Fujinaka, Todd]

I don?t understand ?most defective a bug?. It is a bug.

Most people don?t care too much about using two queues on a controller with only one PCIe lane at 2.5GT/s because the PCIe bus is the bottleneck in this case. In our testing we found that two queues makes little difference in performance and that?s why in our older single-lane 2.5GT/s cards.

We understand that there are more uses for more queues than performance, so we will be fixing this.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka at intel.com
(503) 712-4565

From: Tal Abudi [mailto:talabudi at gmail.com]
Sent: Friday, December 18, 2015 1:49 AM
To: Fujinaka, Todd
Cc: Alex Duyck; intel-wired-lan at lists.osuosl.org; alexander.duyck at gmail.com
Subject: Re: [Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

The I211 supports only 2 queues according to the datasheet.
Is there a real difference between E1000_MRQC_ENABLE_RSS_4Q and E1000_MRQC_ENABLE_RSS_8Q ?
The number of the rss is set inside the adapter and the netdevice's dev.
And we can see in igb_init_queue_configuration() that max_rss_queues = IGB_MAX_RX_QUEUES_I211 (2).

I tested 5.3.2 with this patch on a 2 core system and packets distributed evenly among queues.

This is most defective a bug, right ?

On Thu, Dec 17, 2015 at 8:52 PM, Fujinaka, Todd <todd.fujinaka at intel.com<mailto:todd.fujinaka@intel.com>> wrote:
As I was composing this email, I noticed the same thing. I'll double-check to make sure the comment is actually necessary.

Thanks Alex!

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka at intel.com<mailto:todd.fujinaka@intel.com>
(503) 712-4565<tel:%28503%29%20712-4565>


-----Original Message-----
From: Alex Duyck [mailto:aduyck at mirantis.com<mailto:aduyck@mirantis.com>]
Sent: Thursday, December 17, 2015 10:43 AM
To: Fujinaka, Todd
Cc: intel-wired-lan at lists.osuosl.org<mailto:intel-wired-lan@lists.osuosl.org>; alexander.duyck at gmail.com<mailto:alexander.duyck@gmail.com>; Tal Abudi
Subject: Re: [Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

On Thu, Dec 17, 2015 at 10:37 AM, Fujinaka, Todd <todd.fujinaka at intel.com<mailto:todd.fujinaka@intel.com>> wrote:
> I've spent too much time looking into this. I think this was added when someone looked for the register value and it was the same as E1000_MRQC_ENABLE_RSS_8Q and disabled i211 because it doesn't have 8 queues.
>
> I've been asked if you could spin this to add a comment that the two values are the same (E1000_MRQC_ENABLE_RSS_8Q and E1000_MRQC_ENABLE_RSS_4Q).

That would be even more confusing since the value that is being used is RSS_4Q so I am not sure where the 8Q comes into play.  I guess at some point it was renamed?  Feel free to modify the code however you want.  I was just going through and trying to figure out the issue that had been reported and I noticed it wasn't getting enabled.  It's fine with me if you drop me as author if you guys rewrite the patch and add a comment.  You could probably just add a Suggested-by credit to the patch you generate.

- Alex



--
Best regards,
Tal Abudi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20151218/569b3eb8/attachment.html>

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Alexander Duyck]

On Fri, Dec 18, 2015 at 1:49 AM, Tal Abudi <talabudi@gmail.com> wrote:
> The I211 supports only 2 queues according to the datasheet.
> Is there a real difference between E1000_MRQC_ENABLE_RSS_4Q and
> E1000_MRQC_ENABLE_RSS_8Q ?

I suspect is is just naming.  This should probably be updated to
remove the _XQ entirely.  On 82575 we only supported 4 queues for RSS,
on 82576 it was as much as 16 if I recall correctly.  For 82580, i350,
and i211 I think it is 8, and on i211 it is only 2 but the datasheet
calls it out as RSS_4Q which I believe refers back to the original
82575 documentation.  Since the number varies all over the place we
probably should go through and rename the define.

> The number of the rss is set inside the adapter and the netdevice's dev.
> And we can see in igb_init_queue_configuration() that max_rss_queues =
> IGB_MAX_RX_QUEUES_I211 (2).

Right we only support 2 queues on this hardware, but the datasheet is
calling out 4Q as that is what it was defined in the first iteration
of this driver for the 82575 hardware.  We should have probably
updated this when 82576 was introduced but it looks like the define
was never renamed.

> I tested 5.3.2 with this patch on a 2 core system and packets distributed
> evenly among queues.

Thanks for testing this.  At least we now know that was the issue.

> This is most defective a bug, right ?

This is a bug.  We just need to leave it to the Intel guys to sort out
as I think there may need to be a rename or move of some of the
defines in their initialization code.

- Alex

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Fujinaka, Todd]

Good idea on the renaming Alex. I'll put that on my list.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka at intel.com
(503) 712-4565

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com] 
Sent: Friday, December 18, 2015 7:14 AM
To: Tal Abudi
Cc: Fujinaka, Todd; Alex Duyck; intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

On Fri, Dec 18, 2015 at 1:49 AM, Tal Abudi <talabudi@gmail.com> wrote:
> The I211 supports only 2 queues according to the datasheet.
> Is there a real difference between E1000_MRQC_ENABLE_RSS_4Q and 
> E1000_MRQC_ENABLE_RSS_8Q ?

I suspect is is just naming.  This should probably be updated to remove the _XQ entirely.  On 82575 we only supported 4 queues for RSS, on 82576 it was as much as 16 if I recall correctly.  For 82580, i350, and i211 I think it is 8, and on i211 it is only 2 but the datasheet calls it out as RSS_4Q which I believe refers back to the original
82575 documentation.  Since the number varies all over the place we probably should go through and rename the define.

> The number of the rss is set inside the adapter and the netdevice's dev.
> And we can see in igb_init_queue_configuration() that max_rss_queues =
> IGB_MAX_RX_QUEUES_I211 (2).

Right we only support 2 queues on this hardware, but the datasheet is calling out 4Q as that is what it was defined in the first iteration of this driver for the 82575 hardware.  We should have probably updated this when 82576 was introduced but it looks like the define was never renamed.

> I tested 5.3.2 with this patch on a 2 core system and packets 
> distributed evenly among queues.

Thanks for testing this.  At least we now know that was the issue.

> This is most defective a bug, right ?

This is a bug.  We just need to leave it to the Intel guys to sort out as I think there may need to be a rename or move of some of the defines in their initialization code.

- Alex

[Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter

[Tal Abudi]

> This is most defective a bug, right ?

Todd, Sorry, I meant most definitely a bug.
The condition might imply that this is by-design.

On Fri, Dec 18, 2015 at 5:48 PM, Fujinaka, Todd <todd.fujinaka@intel.com>
wrote:

> Good idea on the renaming Alex. I'll put that on my list.
>
> Todd Fujinaka
> Software Application Engineer
> Networking Division (ND)
> Intel Corporation
> todd.fujinaka at intel.com
> (503) 712-4565
>
> -----Original Message-----
> From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
> Sent: Friday, December 18, 2015 7:14 AM
> To: Tal Abudi
> Cc: Fujinaka, Todd; Alex Duyck; intel-wired-lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH] igb: Enable RSS for i211 adapter
>
> On Fri, Dec 18, 2015 at 1:49 AM, Tal Abudi <talabudi@gmail.com> wrote:
> > The I211 supports only 2 queues according to the datasheet.
> > Is there a real difference between E1000_MRQC_ENABLE_RSS_4Q and
> > E1000_MRQC_ENABLE_RSS_8Q ?
>
> I suspect is is just naming.  This should probably be updated to remove
> the _XQ entirely.  On 82575 we only supported 4 queues for RSS, on 82576 it
> was as much as 16 if I recall correctly.  For 82580, i350, and i211 I think
> it is 8, and on i211 it is only 2 but the datasheet calls it out as RSS_4Q
> which I believe refers back to the original
> 82575 documentation.  Since the number varies all over the place we
> probably should go through and rename the define.
>
> > The number of the rss is set inside the adapter and the netdevice's dev.
> > And we can see in igb_init_queue_configuration() that max_rss_queues =
> > IGB_MAX_RX_QUEUES_I211 (2).
>
> Right we only support 2 queues on this hardware, but the datasheet is
> calling out 4Q as that is what it was defined in the first iteration of
> this driver for the 82575 hardware.  We should have probably updated this
> when 82576 was introduced but it looks like the define was never renamed.
>
> > I tested 5.3.2 with this patch on a 2 core system and packets
> > distributed evenly among queues.
>
> Thanks for testing this.  At least we now know that was the issue.
>
> > This is most defective a bug, right ?
>
> This is a bug.  We just need to leave it to the Intel guys to sort out as
> I think there may need to be a rename or move of some of the defines in
> their initialization code.
>
> - Alex
>



-- 
Best regards,
Tal Abudi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20151218/e2bba2f3/attachment.html>