Patchwork [RFC,net-next,01/14] Add Default

login
register
mail settings
Submitter Yuval Mintz
Date June 19, 2012, 3:13 p.m.
Message ID <1340118848-30978-2-git-send-email-yuvalmin@broadcom.com>
Download mbox | patch
Permalink /patch/165759/
State RFC
Delegated to: David Miller
Headers show

Comments

Yuval Mintz - June 19, 2012, 3:13 p.m.
Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 include/linux/etherdevice.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Alexander Duyck - June 19, 2012, 4:37 p.m.
On 06/19/2012 08:13 AM, Yuval Mintz wrote:
> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
>  include/linux/etherdevice.h |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index 3d406e0..bb1ecaf 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -44,7 +44,10 @@ extern int eth_mac_addr(struct net_device *dev, void *p);
>  extern int eth_change_mtu(struct net_device *dev, int new_mtu);
>  extern int eth_validate_addr(struct net_device *dev);
>  
> -
> +/* The maximal number of RSS queues a driver should have unless configured
> + * so explicitly.
> + */
> +#define DEFAULT_MAX_NUM_RSS_QUEUES (8)
>  
>  extern struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
>  					    unsigned int rxqs);
I'm not a big fan of just having this as a fixed define in the code.  It
seems like it would make much more sense to have this in the Kconfig
somewhere as a range value if you plan on making this changeable in the
future.

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 19, 2012, 5:41 p.m.
On Tue, 2012-06-19 at 09:37 -0700, Alexander Duyck wrote:
> On 06/19/2012 08:13 AM, Yuval Mintz wrote:
> > Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> > Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> > ---
> >  include/linux/etherdevice.h |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > index 3d406e0..bb1ecaf 100644
> > --- a/include/linux/etherdevice.h
> > +++ b/include/linux/etherdevice.h
> > @@ -44,7 +44,10 @@ extern int eth_mac_addr(struct net_device *dev, void *p);
> >  extern int eth_change_mtu(struct net_device *dev, int new_mtu);
> >  extern int eth_validate_addr(struct net_device *dev);
> >  
> > -
> > +/* The maximal number of RSS queues a driver should have unless configured
> > + * so explicitly.
> > + */
> > +#define DEFAULT_MAX_NUM_RSS_QUEUES (8)
> >  
> >  extern struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
> >  					    unsigned int rxqs);
> I'm not a big fan of just having this as a fixed define in the code.  It
> seems like it would make much more sense to have this in the Kconfig
> somewhere as a range value if you plan on making this changeable in the
> future.

My original suggestion was a kernel command line parameter, but Dave was
less than enthusiastic. If you will follow the original thread, you can
probably understand why I decided to adopt Dave's constant approach
without suggesting Kconfig:
http://marc.info/?l=linux-netdev&m=133992386010982&w=2

However, 8 is not a holy number - I'm open for suggestions.

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 19, 2012, 7:01 p.m.
On 06/19/2012 10:41 AM, Eilon Greenstein wrote:
> On Tue, 2012-06-19 at 09:37 -0700, Alexander Duyck wrote:
>> On 06/19/2012 08:13 AM, Yuval Mintz wrote:
>>> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
>>> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
>>> ---
>>>  include/linux/etherdevice.h |    5 ++++-
>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
>>> index 3d406e0..bb1ecaf 100644
>>> --- a/include/linux/etherdevice.h
>>> +++ b/include/linux/etherdevice.h
>>> @@ -44,7 +44,10 @@ extern int eth_mac_addr(struct net_device *dev, void *p);
>>>  extern int eth_change_mtu(struct net_device *dev, int new_mtu);
>>>  extern int eth_validate_addr(struct net_device *dev);
>>>  
>>> -
>>> +/* The maximal number of RSS queues a driver should have unless configured
>>> + * so explicitly.
>>> + */
>>> +#define DEFAULT_MAX_NUM_RSS_QUEUES (8)
>>>  
>>>  extern struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
>>>  					    unsigned int rxqs);
>> I'm not a big fan of just having this as a fixed define in the code.  It
>> seems like it would make much more sense to have this in the Kconfig
>> somewhere as a range value if you plan on making this changeable in the
>> future.
> My original suggestion was a kernel command line parameter, but Dave was
> less than enthusiastic. If you will follow the original thread, you can
> probably understand why I decided to adopt Dave's constant approach
> without suggesting Kconfig:
> http://marc.info/?l=linux-netdev&m=133992386010982&w=2
There is a huge difference between a kernel parameter an a kconfig
value.  The main idea behind the kconfig value is that you are going to
have different preferences depending on architectures and such so it
would make much more sense to have the default as a config option.
> However, 8 is not a holy number - I'm open for suggestions.
>
> Thanks,
> Eilon
I'm not sure why you couldn't just limit it to 16.  From what I can tell
that is the largest number that gets used for RSS queues on almost all
the different hardware out there.

As far as the rest of the patches for the Intel drivers go you might be
better off if you understood how we allocate queues on the ixgbe/ixgbevf
drivers.  Usually we have the number of queues determined before we set
the number of vectors so your patches that limited the number of vectors
aren't going to have the effect you desire.  So for example RSS
configuration is currently handled in either ixgbe_set_rss_queues or
ixgbe_set_dcb_queues depending on the mode the driver is in.  You would
be much better off looking there for how to limit the RSS queueing on
the ixgbe adapter.

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 19, 2012, 7:53 p.m.
On Tue, 2012-06-19 at 12:01 -0700, Alexander Duyck wrote:
> On 06/19/2012 10:41 AM, Eilon Greenstein wrote:
> > On Tue, 2012-06-19 at 09:37 -0700, Alexander Duyck wrote:
> >> On 06/19/2012 08:13 AM, Yuval Mintz wrote:
> >>> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> >>> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> >>> ---
> >>>  include/linux/etherdevice.h |    5 ++++-
> >>>  1 files changed, 4 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> >>> index 3d406e0..bb1ecaf 100644
> >>> --- a/include/linux/etherdevice.h
> >>> +++ b/include/linux/etherdevice.h
> >>> @@ -44,7 +44,10 @@ extern int eth_mac_addr(struct net_device *dev, void *p);
> >>>  extern int eth_change_mtu(struct net_device *dev, int new_mtu);
> >>>  extern int eth_validate_addr(struct net_device *dev);
> >>>  
> >>> -
> >>> +/* The maximal number of RSS queues a driver should have unless configured
> >>> + * so explicitly.
> >>> + */
> >>> +#define DEFAULT_MAX_NUM_RSS_QUEUES (8)
> >>>  
> >>>  extern struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
> >>>  					    unsigned int rxqs);
> >> I'm not a big fan of just having this as a fixed define in the code.  It
> >> seems like it would make much more sense to have this in the Kconfig
> >> somewhere as a range value if you plan on making this changeable in the
> >> future.
> > My original suggestion was a kernel command line parameter, but Dave was
> > less than enthusiastic. If you will follow the original thread, you can
> > probably understand why I decided to adopt Dave's constant approach
> > without suggesting Kconfig:
> > http://marc.info/?l=linux-netdev&m=133992386010982&w=2
> There is a huge difference between a kernel parameter an a kconfig
> value.  The main idea behind the kconfig value is that you are going to
> have different preferences depending on architectures and such so it
> would make much more sense to have the default as a config option.

Yes, I'm aware of that. Coming from the orientation of number of CPUs
and memory constrains, the kernel parameter came to mind first, after
receiving the reply about using just a good default, I have considered
the kconfig alternative but decided not to make further suggestions and
just go with a good default.

> I'm not sure why you couldn't just limit it to 16.  From what I can tell
> that is the largest number that gets used for RSS queues on almost all
> the different hardware out there.

cxgb4 32, myril10ge 32, efx 32, niu 24.
The point is that I was requested by a customer to support more queues,
but simply  enabling that much more MSI-X vectors in the FW will cause
the driver to consume too much memory and this is probably not desired
for most users. Having the set_channels API is good solution to have a
default value which is different than the maximal value, and that brings
us to where we are now - finding a default value for all multi-queue
drivers.


> As far as the rest of the patches for the Intel drivers go you might be
> better off if you understood how we allocate queues on the ixgbe/ixgbevf
> drivers.  Usually we have the number of queues determined before we set
> the number of vectors so your patches that limited the number of vectors
> aren't going to have the effect you desire.  So for example RSS
> configuration is currently handled in either ixgbe_set_rss_queues or
> ixgbe_set_dcb_queues depending on the mode the driver is in.  You would
> be much better off looking there for how to limit the RSS queueing on
> the ixgbe adapter.

OK, we will move the logic to those functions.


--
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
David Miller - June 19, 2012, 9:22 p.m.
From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Tue, 19 Jun 2012 09:37:18 -0700

> I'm not a big fan of just having this as a fixed define in the code.  It
> seems like it would make much more sense to have this in the Kconfig
> somewhere as a range value if you plan on making this changeable in the
> future.

Please not Kconfig :-/
--
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
David Miller - June 19, 2012, 9:26 p.m.
From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Tue, 19 Jun 2012 12:01:54 -0700

> There is a huge difference between a kernel parameter an a kconfig
> value.  The main idea behind the kconfig value is that you are going to
> have different preferences depending on architectures and such so it
> would make much more sense to have the default as a config option.

I don't think the issue of how many queues to use in a network driver
when you have 1024 cpus is architecture specific.

Please drop this idea, thanks.
--
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/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 3d406e0..bb1ecaf 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -44,7 +44,10 @@  extern int eth_mac_addr(struct net_device *dev, void *p);
 extern int eth_change_mtu(struct net_device *dev, int new_mtu);
 extern int eth_validate_addr(struct net_device *dev);
 
-
+/* The maximal number of RSS queues a driver should have unless configured
+ * so explicitly.
+ */
+#define DEFAULT_MAX_NUM_RSS_QUEUES (8)
 
 extern struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
 					    unsigned int rxqs);