| Message ID | 9B4A1B1917080E46B64F07F2989DADD65AAE7871@ORSMSX114.amr.corp.intel.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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@intel.com (503) 712-4565 -----Original Message----- From: Alex Duyck [mailto:aduyck@mirantis.com] Sent: Thursday, December 17, 2015 10:43 AM To: Fujinaka, Todd Cc: intel-wired-lan@lists.osuosl.org; 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@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
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@intel.com > (503) 712-4565 > > > -----Original Message----- > From: Alex Duyck [mailto:aduyck@mirantis.com] > Sent: Thursday, December 17, 2015 10:43 AM > To: Fujinaka, Todd > Cc: intel-wired-lan@lists.osuosl.org; 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@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 >
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@intel.com (503) 712-4565 From: Tal Abudi [mailto:talabudi@gmail.com] Sent: Friday, December 18, 2015 1:49 AM To: Fujinaka, Todd Cc: Alex Duyck; intel-wired-lan@lists.osuosl.org; alexander.duyck@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@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@intel.com<mailto:todd.fujinaka@intel.com> (503) 712-4565<tel:%28503%29%20712-4565> -----Original Message----- From: Alex Duyck [mailto:aduyck@mirantis.com<mailto:aduyck@mirantis.com>] Sent: Thursday, December 17, 2015 10:43 AM To: Fujinaka, Todd Cc: intel-wired-lan@lists.osuosl.org<mailto:intel-wired-lan@lists.osuosl.org>; alexander.duyck@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@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
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
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@intel.com (503) 712-4565 -----Original Message----- From: Alexander Duyck [mailto:alexander.duyck@gmail.com] Sent: Friday, December 18, 2015 7:14 AM To: Tal Abudi Cc: Fujinaka, Todd; Alex Duyck; intel-wired-lan@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
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);