| Message ID | 1340624745-8650-15-git-send-email-yuvalmin@broadcom.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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. */