Patchwork [RFC,net-next,(v2),14/14] ixgbevf: set maximal number of default RSS queues

login
register
mail settings
Submitter Yuval Mintz
Date June 25, 2012, 11:45 a.m.
Message ID <1340624745-8650-15-git-send-email-yuvalmin@broadcom.com>
Download mbox | patch
Permalink /patch/167048/
State RFC
Delegated to: David Miller
Headers show

Comments

Yuval Mintz - June 25, 2012, 11:45 a.m.
Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
Cc: Greg Rose <gregory.v.rose@intel.com>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Alexander Duyck - June 25, 2012, 3:45 p.m.
On 06/25/2012 04:45 AM, Yuval Mintz wrote:
> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> Cc: Greg Rose <gregory.v.rose@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> index f69ec42..7889644 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> @@ -2023,7 +2023,7 @@ static int ixgbevf_set_interrupt_capability(struct ixgbevf_adapter *adapter)
>  	 * (roughly) twice the number of vectors as there are CPU's.
>  	 */
>  	v_budget = min(adapter->num_rx_queues + adapter->num_tx_queues,
> -		       (int)(num_online_cpus() * 2)) + NON_Q_VECTORS;
> +		       netif_get_num_default_rss_queues() * 2) + NON_Q_VECTORS;
>  
>  	/* A failure in MSI-X entry allocation isn't fatal, but it does
>  	 * mean we disable MSI-X capabilities of the adapter. */
I'm not even sure why you are bothering to alter this driver since it is
currently single queue only.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eilon Greenstein - June 25, 2012, 3:53 p.m.
On Mon, 2012-06-25 at 08:45 -0700, Alexander Duyck wrote:
> On 06/25/2012 04:45 AM, Yuval Mintz wrote:
> > Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> > Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> >
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> > Cc: Greg Rose <gregory.v.rose@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > index f69ec42..7889644 100644
> > --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > @@ -2023,7 +2023,7 @@ static int ixgbevf_set_interrupt_capability(struct ixgbevf_adapter *adapter)
> >  	 * (roughly) twice the number of vectors as there are CPU's.
> >  	 */
> >  	v_budget = min(adapter->num_rx_queues + adapter->num_tx_queues,
> > -		       (int)(num_online_cpus() * 2)) + NON_Q_VECTORS;
> > +		       netif_get_num_default_rss_queues() * 2) + NON_Q_VECTORS;
> >  
> >  	/* A failure in MSI-X entry allocation isn't fatal, but it does
> >  	 * mean we disable MSI-X capabilities of the adapter. */
> I'm not even sure why you are bothering to alter this driver since it is
> currently single queue only.

Alex,

Can you please explain the logic behind these lines in the current
implementation?
	v_budget = min(adapter->num_rx_queues + adapter->num_tx_queues,
		       (int)(num_online_cpus() * 2)) + NON_Q_VECTORS;
It looks as if you were trying to be generic and support a future in
which this driver will support more than a single queue - to be future
ready, this RFC is replacing the usage of num_online_cpus with
netif_get_num_default_rss_queues. Are you suggesting to remove these
lines all together? Keeping this minimum function with only the
num_online_cpus does not seem right without considering also the new
default value.

Thanks,
Eilon



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Duyck - June 25, 2012, 4:33 p.m.
On 06/25/2012 08:53 AM, Eilon Greenstein wrote:
> On Mon, 2012-06-25 at 08:45 -0700, Alexander Duyck wrote:
>> On 06/25/2012 04:45 AM, Yuval Mintz wrote:
>>> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
>>> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
>>>
>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
>>> Cc: Greg Rose <gregory.v.rose@intel.com>
>>> ---
>>>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> index f69ec42..7889644 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> @@ -2023,7 +2023,7 @@ static int ixgbevf_set_interrupt_capability(struct ixgbevf_adapter *adapter)
>>>  	 * (roughly) twice the number of vectors as there are CPU's.
>>>  	 */
>>>  	v_budget = min(adapter->num_rx_queues + adapter->num_tx_queues,
>>> -		       (int)(num_online_cpus() * 2)) + NON_Q_VECTORS;
>>> +		       netif_get_num_default_rss_queues() * 2) + NON_Q_VECTORS;
>>>  
>>>  	/* A failure in MSI-X entry allocation isn't fatal, but it does
>>>  	 * mean we disable MSI-X capabilities of the adapter. */
>> I'm not even sure why you are bothering to alter this driver since it is
>> currently single queue only.
> Alex,
>
> Can you please explain the logic behind these lines in the current
> implementation?
> 	v_budget = min(adapter->num_rx_queues + adapter->num_tx_queues,
> 		       (int)(num_online_cpus() * 2)) + NON_Q_VECTORS;
> It looks as if you were trying to be generic and support a future in
> which this driver will support more than a single queue - to be future
> ready, this RFC is replacing the usage of num_online_cpus with
> netif_get_num_default_rss_queues. Are you suggesting to remove these
> lines all together? Keeping this minimum function with only the
> num_online_cpus does not seem right without considering also the new
> default value.
>
> Thanks,
> Eilon
Eilon,

It seems like you didn't listen to anything I told you for the last
round of patches.

You are confusing interrupt vectors with queues as you did with ixgbe. 
The VF driver supports up to 3 interrupts, but is only single queue.  As
such we can split things out so that we either support one vector with
both Tx and Rx or separate Tx and Rx vectors.  How many vectors you have
doesn't really matter anyway since this patch doesn't limit queues.  All
it does is limit the number of interrupt vectors.

I suggest you locate where we set "adapter->num_rx_queues" and attempt
to address the issue there.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eilon Greenstein - June 25, 2012, 6:28 p.m.
On Mon, 2012-06-25 at 09:33 -0700, Alexander Duyck wrote:
> I suggest you locate where we set "adapter->num_rx_queues" and attempt
> to address the issue there.

I see what you mean. On one hand, you use the num_rx_queues as if it can
get higher numbers and the code seems to be ready for that with all the
for loops in place, but on the other hand, it is hard coded to 1 at
ixgbevf_set_num_queues and it seems weird to add min function with
explicit '1'. Do you suggest to leave this driver alone hoping that if
in the future the '1' will change the author will remember to use the
default value?

Thanks,
Eilon



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index f69ec42..7889644 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -2023,7 +2023,7 @@  static int ixgbevf_set_interrupt_capability(struct ixgbevf_adapter *adapter)
 	 * (roughly) twice the number of vectors as there are CPU's.
 	 */
 	v_budget = min(adapter->num_rx_queues + adapter->num_tx_queues,
-		       (int)(num_online_cpus() * 2)) + NON_Q_VECTORS;
+		       netif_get_num_default_rss_queues() * 2) + NON_Q_VECTORS;
 
 	/* A failure in MSI-X entry allocation isn't fatal, but it does
 	 * mean we disable MSI-X capabilities of the adapter. */