[net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[John Fastabend]

The logic for netif_set_real_num_rx_queues is the following,

netif_set_real_num_rx_queues(dev, rxq)
{
	...
	if (dev->reg_state == NETREG_REGISTERED) {
		...
	} else {
		dev->num_rx_queues = rxq;
	}

	dev->real_num_rx_queues = rxq;
	return 0;
}

Some drivers init path looks like the following,

alloc_etherdev_mq(priv_sz, max_num_queues_ever);
...
netif_set_real_num_rx_queues(dev, queues_to_use_now);
...
register_netdev(dev);
...

Because netif_set_real_num_rx_queues sets num_rx_queues if the
reg state is not NETREG_REGISTERED we end up with the incorrect
max number of rx queues. This patch proposes to remove the else
clause above so this does not occur.  Also just reading the
function set_real_num it seems a bit unexpected that num_rx_queues
gets set.

CC: Ben Hutchings <bhutchings@solarflare.com>

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/core/dev.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a313bab..f78d996 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1592,8 +1592,6 @@ int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq)
 						  rxq);
 		if (rc)
 			return rc;
-	} else {
-		dev->num_rx_queues = rxq;
 	}
 
 	dev->real_num_rx_queues = rxq;

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Eric Dumazet]

Le lundi 04 octobre 2010 à 15:00 -0700, John Fastabend a écrit :
> The logic for netif_set_real_num_rx_queues is the following,
> 
> netif_set_real_num_rx_queues(dev, rxq)
> {
> 	...
> 	if (dev->reg_state == NETREG_REGISTERED) {
> 		...
> 	} else {
> 		dev->num_rx_queues = rxq;
> 	}
> 
> 	dev->real_num_rx_queues = rxq;
> 	return 0;
> }
> 
> Some drivers init path looks like the following,
> 
> alloc_etherdev_mq(priv_sz, max_num_queues_ever);
> ...
> netif_set_real_num_rx_queues(dev, queues_to_use_now);
> ...
> register_netdev(dev);
> ...
> 
> Because netif_set_real_num_rx_queues sets num_rx_queues if the
> reg state is not NETREG_REGISTERED we end up with the incorrect
> max number of rx queues. This patch proposes to remove the else
> clause above so this does not occur.  Also just reading the
> function set_real_num it seems a bit unexpected that num_rx_queues
> gets set.
> 

You dont tell why its "incorrect".

Why should we keep num_rx_queues > real_num_rx_queues ?

It creates /sys files, and Qdisc stuff for nothing...



> CC: Ben Hutchings <bhutchings@solarflare.com>
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  net/core/dev.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a313bab..f78d996 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1592,8 +1592,6 @@ int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq)
>  						  rxq);
>  		if (rc)
>  			return rc;
> -	} else {
> -		dev->num_rx_queues = rxq;
>  	}
>  
>  	dev->real_num_rx_queues = rxq;
> 
> --

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[John Fastabend]

On 10/4/2010 10:35 PM, Eric Dumazet wrote:
> Le lundi 04 octobre 2010 à 15:00 -0700, John Fastabend a écrit :
>> The logic for netif_set_real_num_rx_queues is the following,
>>
>> netif_set_real_num_rx_queues(dev, rxq)
>> {
>> 	...
>> 	if (dev->reg_state == NETREG_REGISTERED) {
>> 		...
>> 	} else {
>> 		dev->num_rx_queues = rxq;
>> 	}
>>
>> 	dev->real_num_rx_queues = rxq;
>> 	return 0;
>> }
>>
>> Some drivers init path looks like the following,
>>
>> alloc_etherdev_mq(priv_sz, max_num_queues_ever);
>> ...
>> netif_set_real_num_rx_queues(dev, queues_to_use_now);
>> ...
>> register_netdev(dev);
>> ...
>>
>> Because netif_set_real_num_rx_queues sets num_rx_queues if the
>> reg state is not NETREG_REGISTERED we end up with the incorrect
>> max number of rx queues. This patch proposes to remove the else
>> clause above so this does not occur.  Also just reading the
>> function set_real_num it seems a bit unexpected that num_rx_queues
>> gets set.
>>
> 
> You dont tell why its "incorrect".
> 

OK that is a poor description.

> Why should we keep num_rx_queues > real_num_rx_queues ?
> 

If we do not ever need them then we should not keep them I agree.
But having netif_set_real_num_rx_queues set something other then
'real_num_rx_queues' does not seem right to me at least. Also
netif_set_real_num_tx_queues and netif_set_real_num_rx_queues have
different behavior. It would be nice if this weren't the case but
they allocate queues in two places.

The drivers Ben modified seem to be split between these two flows

alloc_etherdev_mq(priv_sz, max_num_queues_ever);
register_netdev(dev);
netif_set_real_num_rx_queues(dev, queues_to_use_now);

and

alloc_etherdev_mq(priv_sz, max_num_queues_ever);
netif_set_real_num_rx_queues(dev, queues_to_use_now);
register_netdev(dev);

In the first case we never set 'num_rx_queues' because its after
the register so that leaves the second case. All the remaining
drivers except ixgbe, igb, and myri10ge know or could easily find
out the number of rx queues at alloc_etherdev_mq and are really trying
to explicitly set the num_rx_queues. Adding a num_rx_queues param to
alloc_etherdev_mq might make more sense in this case.

The myri10ge is querying the firmware which seems to be tangled up with
the netdev priv space, but otherwise isn't using any new knowledge. And
ixgbe/igb may end up capping the max number of queues because it is
trying to set the number of queues it intends to use now not the max it
will ever consume.

My point with this patch is I do not see any cases where we really do
not know the max number rx queues until after alloc_etherdev_mq() but before
register_netdev(). The piece I missed is if a driver wants to set rx queues
and tx queues separately they either need to explicitly set rx_num_queues or
use netif_set_real_num_rx_queues before registering the netdev. This syntax
seems error prone to me, and it would be better to set this in alloc_etherdev_mq().
But, maybe you disagree?

So I can either go rework the ixgbe/igb init flow so it does what I want.
Or add a parameter to alloc_etherdev_mq for rx queues, remove the
netif_set_real_num_rx_queues where it is not needed and modify the drivers
to use this interface. I like the second case because it makes the
netif_set_real_num_[rx|tx}_queues() behave the same but could do the first
just as easily.

Thanks,
John 

> It creates /sys files, and Qdisc stuff for nothing...
> 
> 
> 
>> CC: Ben Hutchings <bhutchings@solarflare.com>
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>
>>  net/core/dev.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index a313bab..f78d996 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1592,8 +1592,6 @@ int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq)
>>  						  rxq);
>>  		if (rc)
>>  			return rc;
>> -	} else {
>> -		dev->num_rx_queues = rxq;
>>  	}
>>  
>>  	dev->real_num_rx_queues = rxq;
>>
>> --
> 
> 

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Ben Hutchings]

On Tue, 2010-10-05 at 09:08 -0700, John Fastabend wrote:
> On 10/4/2010 10:35 PM, Eric Dumazet wrote:
> > Le lundi 04 octobre 2010 à 15:00 -0700, John Fastabend a écrit :
> >> The logic for netif_set_real_num_rx_queues is the following,
> >>
> >> netif_set_real_num_rx_queues(dev, rxq)
> >> {
> >> 	...
> >> 	if (dev->reg_state == NETREG_REGISTERED) {
> >> 		...
> >> 	} else {
> >> 		dev->num_rx_queues = rxq;
> >> 	}
> >>
> >> 	dev->real_num_rx_queues = rxq;
> >> 	return 0;
> >> }
> >>
> >> Some drivers init path looks like the following,
> >>
> >> alloc_etherdev_mq(priv_sz, max_num_queues_ever);
> >> ...
> >> netif_set_real_num_rx_queues(dev, queues_to_use_now);
> >> ...
> >> register_netdev(dev);
> >> ...
> >>
> >> Because netif_set_real_num_rx_queues sets num_rx_queues if the
> >> reg state is not NETREG_REGISTERED we end up with the incorrect
> >> max number of rx queues. This patch proposes to remove the else
> >> clause above so this does not occur.  Also just reading the
> >> function set_real_num it seems a bit unexpected that num_rx_queues
> >> gets set.
> >>
> > 
> > You dont tell why its "incorrect".
> > 
> 
> OK that is a poor description.
> 
> > Why should we keep num_rx_queues > real_num_rx_queues ?
> > 
> 
> If we do not ever need them then we should not keep them I agree.
> But having netif_set_real_num_rx_queues set something other then
> 'real_num_rx_queues' does not seem right to me at least. Also
> netif_set_real_num_tx_queues and netif_set_real_num_rx_queues have
> different behavior. It would be nice if this weren't the case but
> they allocate queues in two places.
[...]

I only did this to satisfy Eric's desire to reduce memory usage.
However, I believe that there are currently no drivers that dynamically
increase numbers of RX or TX queues.  Until there are, there is not much
point in removing this assignment to num_rx_queues.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[John Fastabend]

On 10/5/2010 9:34 AM, Ben Hutchings wrote:
> On Tue, 2010-10-05 at 09:08 -0700, John Fastabend wrote:
>> On 10/4/2010 10:35 PM, Eric Dumazet wrote:
>>> Le lundi 04 octobre 2010 à 15:00 -0700, John Fastabend a écrit :
>>>> The logic for netif_set_real_num_rx_queues is the following,
>>>>
>>>> netif_set_real_num_rx_queues(dev, rxq)
>>>> {
>>>> 	...
>>>> 	if (dev->reg_state == NETREG_REGISTERED) {
>>>> 		...
>>>> 	} else {
>>>> 		dev->num_rx_queues = rxq;
>>>> 	}
>>>>
>>>> 	dev->real_num_rx_queues = rxq;
>>>> 	return 0;
>>>> }
>>>>
>>>> Some drivers init path looks like the following,
>>>>
>>>> alloc_etherdev_mq(priv_sz, max_num_queues_ever);
>>>> ...
>>>> netif_set_real_num_rx_queues(dev, queues_to_use_now);
>>>> ...
>>>> register_netdev(dev);
>>>> ...
>>>>
>>>> Because netif_set_real_num_rx_queues sets num_rx_queues if the
>>>> reg state is not NETREG_REGISTERED we end up with the incorrect
>>>> max number of rx queues. This patch proposes to remove the else
>>>> clause above so this does not occur.  Also just reading the
>>>> function set_real_num it seems a bit unexpected that num_rx_queues
>>>> gets set.
>>>>
>>>
>>> You dont tell why its "incorrect".
>>>
>>
>> OK that is a poor description.
>>
>>> Why should we keep num_rx_queues > real_num_rx_queues ?
>>>
>>
>> If we do not ever need them then we should not keep them I agree.
>> But having netif_set_real_num_rx_queues set something other then
>> 'real_num_rx_queues' does not seem right to me at least. Also
>> netif_set_real_num_tx_queues and netif_set_real_num_rx_queues have
>> different behavior. It would be nice if this weren't the case but
>> they allocate queues in two places.
> [...]
> 
> I only did this to satisfy Eric's desire to reduce memory usage.
> However, I believe that there are currently no drivers that dynamically
> increase numbers of RX or TX queues.  Until there are, there is not much
> point in removing this assignment to num_rx_queues.
> 
> Ben.
> 

ixgbe increases the real_num_[rx|tx]_queues when FCoE or DCB is enabled.
Also many of the drivers could increase the number of queues if they were
given more interrupt vectors at some point. 

But, it is easy enough to patch ixgbe to not set the number of RX queues
currently in use until after the device is registered. Which brings it
inline with many of the other drivers. And then the drivers that are using
it to explicitly set num_rx_queues do not need to change.

Thanks,
John

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[John Fastabend]

On 10/5/2010 10:45 AM, John Fastabend wrote:
> On 10/5/2010 9:34 AM, Ben Hutchings wrote:
>> On Tue, 2010-10-05 at 09:08 -0700, John Fastabend wrote:
>>> On 10/4/2010 10:35 PM, Eric Dumazet wrote:
>>>> Le lundi 04 octobre 2010 à 15:00 -0700, John Fastabend a écrit :
>>>>> The logic for netif_set_real_num_rx_queues is the following,
>>>>>
>>>>> netif_set_real_num_rx_queues(dev, rxq)
>>>>> {
>>>>> 	...
>>>>> 	if (dev->reg_state == NETREG_REGISTERED) {
>>>>> 		...
>>>>> 	} else {
>>>>> 		dev->num_rx_queues = rxq;
>>>>> 	}
>>>>>
>>>>> 	dev->real_num_rx_queues = rxq;
>>>>> 	return 0;
>>>>> }
>>>>>
>>>>> Some drivers init path looks like the following,
>>>>>
>>>>> alloc_etherdev_mq(priv_sz, max_num_queues_ever);
>>>>> ...
>>>>> netif_set_real_num_rx_queues(dev, queues_to_use_now);
>>>>> ...
>>>>> register_netdev(dev);
>>>>> ...
>>>>>
>>>>> Because netif_set_real_num_rx_queues sets num_rx_queues if the
>>>>> reg state is not NETREG_REGISTERED we end up with the incorrect
>>>>> max number of rx queues. This patch proposes to remove the else
>>>>> clause above so this does not occur.  Also just reading the
>>>>> function set_real_num it seems a bit unexpected that num_rx_queues
>>>>> gets set.
>>>>>
>>>>
>>>> You dont tell why its "incorrect".
>>>>
>>>
>>> OK that is a poor description.
>>>
>>>> Why should we keep num_rx_queues > real_num_rx_queues ?
>>>>
>>>
>>> If we do not ever need them then we should not keep them I agree.
>>> But having netif_set_real_num_rx_queues set something other then
>>> 'real_num_rx_queues' does not seem right to me at least. Also
>>> netif_set_real_num_tx_queues and netif_set_real_num_rx_queues have
>>> different behavior. It would be nice if this weren't the case but
>>> they allocate queues in two places.
>> [...]
>>
>> I only did this to satisfy Eric's desire to reduce memory usage.
>> However, I believe that there are currently no drivers that dynamically
>> increase numbers of RX or TX queues.  Until there are, there is not much
>> point in removing this assignment to num_rx_queues.
>>
>> Ben.
>>
> 
> ixgbe increases the real_num_[rx|tx]_queues when FCoE or DCB is enabled.
> Also many of the drivers could increase the number of queues if they were
> given more interrupt vectors at some point.


If I update the handful drivers that use netif_set_real_num_rx_queues()
before the netdevice is registered to explicitly set num_rx_queues this
would address Eric's concerns and fix drivers that really only want to set
real_num_rx_queue.

Any thoughts?

-- John

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Ben Hutchings]

On Wed, 2010-10-06 at 07:52 -0700, John Fastabend wrote:
> On 10/5/2010 10:45 AM, John Fastabend wrote:
> > On 10/5/2010 9:34 AM, Ben Hutchings wrote:
> >> On Tue, 2010-10-05 at 09:08 -0700, John Fastabend wrote:
> >>> On 10/4/2010 10:35 PM, Eric Dumazet wrote:
[...]
> >>>> Why should we keep num_rx_queues > real_num_rx_queues ?
> >>>>
> >>>
> >>> If we do not ever need them then we should not keep them I agree.
> >>> But having netif_set_real_num_rx_queues set something other then
> >>> 'real_num_rx_queues' does not seem right to me at least. Also
> >>> netif_set_real_num_tx_queues and netif_set_real_num_rx_queues have
> >>> different behavior. It would be nice if this weren't the case but
> >>> they allocate queues in two places.
> >> [...]
> >>
> >> I only did this to satisfy Eric's desire to reduce memory usage.
> >> However, I believe that there are currently no drivers that dynamically
> >> increase numbers of RX or TX queues.  Until there are, there is not much
> >> point in removing this assignment to num_rx_queues.
> >>
> >> Ben.
> >>
> > 
> > ixgbe increases the real_num_[rx|tx]_queues when FCoE or DCB is enabled.
> > Also many of the drivers could increase the number of queues if they were
> > given more interrupt vectors at some point.
> 
> 
> If I update the handful drivers that use netif_set_real_num_rx_queues()
> before the netdevice is registered to explicitly set num_rx_queues this
> would address Eric's concerns and fix drivers that really only want to set
> real_num_rx_queue.
> 
> Any thoughts?

Don't add assignments to num_rx_queues.  If it's useful to increase the
number of RX queues later then just remove the assignment to
num_rx_queues from netif_set_real_num_rx_queues() and be done with it.
The waste of memory is minimal now that we only allocate kobjects for
real_num_rx_queues.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[John Fastabend]

On 10/6/2010 8:07 AM, Ben Hutchings wrote:
> On Wed, 2010-10-06 at 07:52 -0700, John Fastabend wrote:
>> On 10/5/2010 10:45 AM, John Fastabend wrote:
>>> On 10/5/2010 9:34 AM, Ben Hutchings wrote:
>>>> On Tue, 2010-10-05 at 09:08 -0700, John Fastabend wrote:
>>>>> On 10/4/2010 10:35 PM, Eric Dumazet wrote:
> [...]
>>>>>> Why should we keep num_rx_queues > real_num_rx_queues ?
>>>>>>
>>>>>
>>>>> If we do not ever need them then we should not keep them I agree.
>>>>> But having netif_set_real_num_rx_queues set something other then
>>>>> 'real_num_rx_queues' does not seem right to me at least. Also
>>>>> netif_set_real_num_tx_queues and netif_set_real_num_rx_queues have
>>>>> different behavior. It would be nice if this weren't the case but
>>>>> they allocate queues in two places.
>>>> [...]
>>>>
>>>> I only did this to satisfy Eric's desire to reduce memory usage.
>>>> However, I believe that there are currently no drivers that dynamically
>>>> increase numbers of RX or TX queues.  Until there are, there is not much
>>>> point in removing this assignment to num_rx_queues.
>>>>
>>>> Ben.
>>>>
>>>
>>> ixgbe increases the real_num_[rx|tx]_queues when FCoE or DCB is enabled.
>>> Also many of the drivers could increase the number of queues if they were
>>> given more interrupt vectors at some point.
>>
>>
>> If I update the handful drivers that use netif_set_real_num_rx_queues()
>> before the netdevice is registered to explicitly set num_rx_queues this
>> would address Eric's concerns and fix drivers that really only want to set
>> real_num_rx_queue.
>>
>> Any thoughts?
> 
> Don't add assignments to num_rx_queues.  If it's useful to increase the
> number of RX queues later then just remove the assignment to
> num_rx_queues from netif_set_real_num_rx_queues() and be done with it.
> The waste of memory is minimal now that we only allocate kobjects for
> real_num_rx_queues.
> 
> Ben.
> 

OK Thanks Ben. I will get a better description on this and resend it.

John.

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Eric Dumazet]

Le mercredi 06 octobre 2010 à 16:07 +0100, Ben Hutchings a écrit :

> The waste of memory is minimal now that we only allocate kobjects for
> real_num_rx_queues.

Thats strange, here with tg3 (and mono queue device) :

0a:04.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5715S
Gigabit Ethernet (rev a3)

grep . /sys/class/net/eth2/queues/rx-*/rps_cpus
/sys/class/net/eth2/queues/rx-0/rps_cpus:00000000
/sys/class/net/eth2/queues/rx-1/rps_cpus:00000000
/sys/class/net/eth2/queues/rx-2/rps_cpus:00000000
/sys/class/net/eth2/queues/rx-3/rps_cpus:00000000
/sys/class/net/eth2/queues/rx-4/rps_cpus:00000000

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Ben Hutchings]

On Wed, 2010-10-06 at 17:23 +0200, Eric Dumazet wrote:
> Le mercredi 06 octobre 2010 à 16:07 +0100, Ben Hutchings a écrit :
> 
> > The waste of memory is minimal now that we only allocate kobjects for
> > real_num_rx_queues.
> 
> Thats strange, here with tg3 (and mono queue device) :
> 
> 0a:04.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5715S
> Gigabit Ethernet (rev a3)
> 
> grep . /sys/class/net/eth2/queues/rx-*/rps_cpus
> /sys/class/net/eth2/queues/rx-0/rps_cpus:00000000
> /sys/class/net/eth2/queues/rx-1/rps_cpus:00000000
> /sys/class/net/eth2/queues/rx-2/rps_cpus:00000000
> /sys/class/net/eth2/queues/rx-3/rps_cpus:00000000
> /sys/class/net/eth2/queues/rx-4/rps_cpus:00000000

It looks like I missed a necessary call to
netif_set_real_num_rx_queues() in tg3.  I suggest that Matt should check
and correct this since I got the numbers wrong last time.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Matt Carlson]

On Wed, Oct 06, 2010 at 08:31:45AM -0700, Ben Hutchings wrote:
> On Wed, 2010-10-06 at 17:23 +0200, Eric Dumazet wrote:
> > Le mercredi 06 octobre 2010 ?? 16:07 +0100, Ben Hutchings a ??crit :
> > 
> > > The waste of memory is minimal now that we only allocate kobjects for
> > > real_num_rx_queues.
> > 
> > Thats strange, here with tg3 (and mono queue device) :
> > 
> > 0a:04.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5715S
> > Gigabit Ethernet (rev a3)
> > 
> > grep . /sys/class/net/eth2/queues/rx-*/rps_cpus
> > /sys/class/net/eth2/queues/rx-0/rps_cpus:00000000
> > /sys/class/net/eth2/queues/rx-1/rps_cpus:00000000
> > /sys/class/net/eth2/queues/rx-2/rps_cpus:00000000
> > /sys/class/net/eth2/queues/rx-3/rps_cpus:00000000
> > /sys/class/net/eth2/queues/rx-4/rps_cpus:00000000
> 
> It looks like I missed a necessary call to
> netif_set_real_num_rx_queues() in tg3.  I suggest that Matt should check
> and correct this since I got the numbers wrong last time.

Yes.  We were missing a call to this function in the legacy case.


[PATCH net-next] tg3: Set real_num_rx_queues for non-multiq devs

Commit 2ddaad397c47de012dfb956b0c05540da1a0dde5 entitled "tg3: Use
netif_set_real_num_{rx,tx}_queues()" added a new call to
netif_set_real_num_rx_queues in tg3_enable_msix().  This call also needs
to be added to the legacy path to correctly reflect the actual number of
rx queues.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
---
 drivers/net/tg3.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 16e1a95..e5b9ec5 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -8906,6 +8906,7 @@ defcfg:
 		tp->irq_cnt = 1;
 		tp->napi[0].irq_vec = tp->pdev->irq;
 		netif_set_real_num_tx_queues(tp->dev, 1);
+		netif_set_real_num_rx_queues(tp->dev, 1);
 	}
 }
 
-- 
1.7.2.2

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Eric Dumazet]

Le mercredi 06 octobre 2010 à 11:14 -0700, Matt Carlson a écrit :

> Yes.  We were missing a call to this function in the legacy case.
> 
> 
> [PATCH net-next] tg3: Set real_num_rx_queues for non-multiq devs
> 
> Commit 2ddaad397c47de012dfb956b0c05540da1a0dde5 entitled "tg3: Use
> netif_set_real_num_{rx,tx}_queues()" added a new call to
> netif_set_real_num_rx_queues in tg3_enable_msix().  This call also needs
> to be added to the legacy path to correctly reflect the actual number of
> rx queues.
> 
> Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> ---
>  drivers/net/tg3.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 16e1a95..e5b9ec5 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -8906,6 +8906,7 @@ defcfg:
>  		tp->irq_cnt = 1;
>  		tp->napi[0].irq_vec = tp->pdev->irq;
>  		netif_set_real_num_tx_queues(tp->dev, 1);
> +		netif_set_real_num_rx_queues(tp->dev, 1);
>  	}
>  }
>  

Thanks, this fixed the thing for me, once device is UP

# insmod drivers/net/tg3.ko
# grep . /sys/class/net/eth3/queues/*/rps_cpus
/sys/class/net/eth3/queues/rx-0/rps_cpus:00000000
/sys/class/net/eth3/queues/rx-1/rps_cpus:00000000
/sys/class/net/eth3/queues/rx-2/rps_cpus:00000000
/sys/class/net/eth3/queues/rx-3/rps_cpus:00000000
/sys/class/net/eth3/queues/rx-4/rps_cpus:00000000
# echo ff >/sys/class/net/eth3/queues/rx-2/rps_cpus
# grep . /sys/class/net/eth3/queues/*/rps_cpus
/sys/class/net/eth3/queues/rx-0/rps_cpus:00000000
/sys/class/net/eth3/queues/rx-1/rps_cpus:00000000
/sys/class/net/eth3/queues/rx-2/rps_cpus:000000ff
/sys/class/net/eth3/queues/rx-3/rps_cpus:00000000
/sys/class/net/eth3/queues/rx-4/rps_cpus:00000000
# ifconfig eth3 up
# grep . /sys/class/net/eth3/queues/*/rps_cpus
00000000

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 06 Oct 2010 20:49:05 +0200

> Le mercredi 06 octobre 2010 à 11:14 -0700, Matt Carlson a écrit :
> 
>> Yes.  We were missing a call to this function in the legacy case.
>> 
>> 
>> [PATCH net-next] tg3: Set real_num_rx_queues for non-multiq devs
>> 
>> Commit 2ddaad397c47de012dfb956b0c05540da1a0dde5 entitled "tg3: Use
>> netif_set_real_num_{rx,tx}_queues()" added a new call to
>> netif_set_real_num_rx_queues in tg3_enable_msix().  This call also needs
>> to be added to the legacy path to correctly reflect the actual number of
>> rx queues.
>> 
>> Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
...
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks everyone.

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Eric Dumazet]

Le mercredi 06 octobre 2010 à 07:52 -0700, John Fastabend a écrit :

> If I update the handful drivers that use netif_set_real_num_rx_queues()
> before the netdevice is registered to explicitly set num_rx_queues this
> would address Eric's concerns and fix drivers that really only want to set
> real_num_rx_queue.
> 

John, it seems current API is not very clean/orthogonal.

If you believe a driver can increase real_num_rx_queue, then we should
apply your patch, and refine API if necessary.

We now allocate rx queues in register_netdevice(), maybe we should do
the same for tx queues.

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[Eric Dumazet]

Le lundi 04 octobre 2010 à 15:00 -0700, John Fastabend a écrit :
> The logic for netif_set_real_num_rx_queues is the following,
> 


...

> Because netif_set_real_num_rx_queues sets num_rx_queues if the
> reg state is not NETREG_REGISTERED we end up with the incorrect
> max number of rx queues. This patch proposes to remove the else
> clause above so this does not occur.  Also just reading the
> function set_real_num it seems a bit unexpected that num_rx_queues
> gets set.
> 
> CC: Ben Hutchings <bhutchings@solarflare.com>
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [net-next-2.6 PATCH] net: netif_set_real_num_rx_queues may cap num_rx_queues at init time

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 07 Oct 2010 08:16:39 +0200

> Le lundi 04 octobre 2010 à 15:00 -0700, John Fastabend a écrit :
>> The logic for netif_set_real_num_rx_queues is the following,
>> 
> 
> 
> ...
> 
>> Because netif_set_real_num_rx_queues sets num_rx_queues if the
>> reg state is not NETREG_REGISTERED we end up with the incorrect
>> max number of rx queues. This patch proposes to remove the else
>> clause above so this does not occur.  Also just reading the
>> function set_real_num it seems a bit unexpected that num_rx_queues
>> gets set.
>> 
>> CC: Ben Hutchings <bhutchings@solarflare.com>
>> 
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks everyone.