RFS seems to have incompatiblities with bridged vlans

RFS seems to have incompatiblities with bridged vlans

[Peter Lieven]

Hi,

i today tried out 2.6.32-rc2 and I see a lot of warning messages like this:

Jun  7 22:33:15 172.21.55.20 kernel: [ 3012.575884] br141 received packet on queue 4, but number of RX queues is 1

The host is a SMP system with 8 cores, so I think there is expected to be one rx queue per CPU, but it seems
the bridge iface has only one.

The br141 config looks like this:

#brctl show br141
bridge name	bridge id		STP enabled	interfaces
br141		8000.001018593cdc	no		eth2.141
							tap0
							tap2

The bridge is basically a bridge between a 802.1q vlan at eth2 and some tap Interfaces bound to
virtual machines on the system.

Peter.

Re: RFS seems to have incompatiblities with bridged vlans

[Stephen Hemminger]

On Mon, 7 Jun 2010 22:36:11 +0200
Peter Lieven <pl@dlh.net> wrote:

> Hi,
> 
> i today tried out 2.6.32-rc2 and I see a lot of warning messages like this:
> 
> Jun  7 22:33:15 172.21.55.20 kernel: [ 3012.575884] br141 received packet on queue 4, but number of RX queues is 1
> 
> The host is a SMP system with 8 cores, so I think there is expected to be one rx queue per CPU, but it seems
> the bridge iface has only one.
> 

The bridge interface has no queues. It doesn't queue any packets. 
The test in receive packet path is not appropriate in this case.
Not sure what the right fix is. Pretending the bridge device has
multiple queues (num_queues == NUM_CPUS) is a possibility but
seems like overhead without real increase in parallelism.

Re: RFS seems to have incompatiblities with bridged vlans

[Eric Dumazet]

Le lundi 07 juin 2010 à 14:59 -0700, Stephen Hemminger a écrit :
> On Mon, 7 Jun 2010 22:36:11 +0200
> Peter Lieven <pl@dlh.net> wrote:
> 
> > Hi,
> > 
> > i today tried out 2.6.32-rc2 and I see a lot of warning messages like this:
> > 
> > Jun  7 22:33:15 172.21.55.20 kernel: [ 3012.575884] br141 received packet on queue 4, but number of RX queues is 1
> > 
> > The host is a SMP system with 8 cores, so I think there is expected to be one rx queue per CPU, but it seems
> > the bridge iface has only one.
> > 
> 
> The bridge interface has no queues. It doesn't queue any packets. 
> The test in receive packet path is not appropriate in this case.
> Not sure what the right fix is. Pretending the bridge device has
> multiple queues (num_queues == NUM_CPUS) is a possibility but
> seems like overhead without real increase in parallelism.

We have same problem with bonding and multiqueue devices...

It does make sense to pretend we have several queues, in case we have
another stage like

eth0	+
  	+-> bond0  -> vlan.1000
eth1	+

because in this case, vlan.1000 would also be multiqueue, so tx will be
parallelized on several queues.

Re: RFS seems to have incompatiblities with bridged vlans

[John Fastabend]

Stephen Hemminger wrote:
> On Mon, 7 Jun 2010 22:36:11 +0200
> Peter Lieven <pl@dlh.net> wrote:
> 
>> Hi,
>>
>> i today tried out 2.6.32-rc2 and I see a lot of warning messages like this:
>>
>> Jun  7 22:33:15 172.21.55.20 kernel: [ 3012.575884] br141 received packet on queue 4, but number of RX queues is 1
>>
>> The host is a SMP system with 8 cores, so I think there is expected to be one rx queue per CPU, but it seems
>> the bridge iface has only one.
>>
> 
> The bridge interface has no queues. It doesn't queue any packets. 
> The test in receive packet path is not appropriate in this case.
> Not sure what the right fix is. Pretending the bridge device has
> multiple queues (num_queues == NUM_CPUS) is a possibility but
> seems like overhead without real increase in parallelism.
> 
> 
> 

There is always a possibility that the underlying device sets the 
queue_mapping to be greater then num_cpus.  Also I suspect the same 
issue exists with bonding devices.  Maybe something like the following 
is worth while? compile tested only,

[PATCH] 8021q: vlan reassigns dev without check queue_mapping

recv path reassigns skb->dev without sanity checking the
queue_mapping field.  This can result in the queue_mapping
field being set incorrectly if the new dev supports less
queues then the underlying device.

This patch just resets the queue_mapping to 0 which should
resolve this issue?  Any thoughts?

The same issue could happen on bonding devices as well.

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

  net/8021q/vlan_core.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index bd537fc..ad309f8 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct 
vlan_group *grp,
  	if (!skb->dev)
  		goto drop;

+	if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
+		skb_set_queue_mapping(skb, 0);
+
  	return (polling ? netif_receive_skb(skb) : netif_rx(skb));

  drop:
@@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct 
vlan_group *grp,
  	if (!skb->dev)
  		goto drop;

+	if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
+		skb_set_queue_mapping(skb, 0);
+
  	for (p = napi->gro_list; p; p = p->next) {
  		NAPI_GRO_CB(p)->same_flow =
  			p->dev == skb->dev && !compare_ether_header(

Re: RFS seems to have incompatiblities with bridged vlans

[John Fastabend]

John Fastabend wrote:
> Stephen Hemminger wrote:
>> On Mon, 7 Jun 2010 22:36:11 +0200
>> Peter Lieven <pl@dlh.net> wrote:
>>
>>> Hi,
>>>
>>> i today tried out 2.6.32-rc2 and I see a lot of warning messages like this:
>>>
>>> Jun  7 22:33:15 172.21.55.20 kernel: [ 3012.575884] br141 received packet on queue 4, but number of RX queues is 1
>>>
>>> The host is a SMP system with 8 cores, so I think there is expected to be one rx queue per CPU, but it seems
>>> the bridge iface has only one.
>>>
>> The bridge interface has no queues. It doesn't queue any packets. 
>> The test in receive packet path is not appropriate in this case.
>> Not sure what the right fix is. Pretending the bridge device has
>> multiple queues (num_queues == NUM_CPUS) is a possibility but
>> seems like overhead without real increase in parallelism.
>>
>>
>>
> 
> There is always a possibility that the underlying device sets the 
> queue_mapping to be greater then num_cpus.  Also I suspect the same 
> issue exists with bonding devices.  Maybe something like the following 
> is worth while? compile tested only,
> 
> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
> 
> recv path reassigns skb->dev without sanity checking the
> queue_mapping field.  This can result in the queue_mapping
> field being set incorrectly if the new dev supports less
> queues then the underlying device.
> 
> This patch just resets the queue_mapping to 0 which should
> resolve this issue?  Any thoughts?
> 
> The same issue could happen on bonding devices as well.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>   net/8021q/vlan_core.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index bd537fc..ad309f8 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct 
> vlan_group *grp,
>   	if (!skb->dev)
>   		goto drop;
> 
> +	if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
> +		skb_set_queue_mapping(skb, 0);
> +

Actually this should be dev->num_rx_queues not real_num_tx_queues.

>   	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> 
>   drop:
> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct 
> vlan_group *grp,
>   	if (!skb->dev)
>   		goto drop;
> 
> +	if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
> +		skb_set_queue_mapping(skb, 0);
> +

same here.

>   	for (p = napi->gro_list; p; p = p->next) {
>   		NAPI_GRO_CB(p)->same_flow =
>   			p->dev == skb->dev && !compare_ether_header(
> --
> 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

Re: RFS seems to have incompatiblities with bridged vlans

[Eric Dumazet]

Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :

> There is always a possibility that the underlying device sets the 
> queue_mapping to be greater then num_cpus.  Also I suspect the same 
> issue exists with bonding devices.  Maybe something like the following 
> is worth while? compile tested only,
> 
> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
> 
> recv path reassigns skb->dev without sanity checking the
> queue_mapping field.  This can result in the queue_mapping
> field being set incorrectly if the new dev supports less
> queues then the underlying device.
> 
> This patch just resets the queue_mapping to 0 which should
> resolve this issue?  Any thoughts?
> 
> The same issue could happen on bonding devices as well.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>   net/8021q/vlan_core.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index bd537fc..ad309f8 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct 
> vlan_group *grp,
>   	if (!skb->dev)
>   		goto drop;
> 
> +	if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
> +		skb_set_queue_mapping(skb, 0);
> +
>   	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> 
>   drop:
> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct 
> vlan_group *grp,
>   	if (!skb->dev)
>   		goto drop;
> 
> +	if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
> +		skb_set_queue_mapping(skb, 0);
> +
>   	for (p = napi->gro_list; p; p = p->next) {
>   		NAPI_GRO_CB(p)->same_flow =
>   			p->dev == skb->dev && !compare_ether_header(
> --

Only a workaround, added in hot path in a otherwise 'good' driver
(multiqueue enabled and ready)

eth0  -------> bond / bridge ---------> vlan.id
(nbtxq=8)      (ntxbq=1)        (nbtxq=X)

X is capped to 1 because of bond/bridge, while bond has no "queue"
(LLTX driver)

Solutions :

1) queue_mapping could be silently tested in get_rps_cpu()...

diff --git a/net/core/dev.c b/net/core/dev.c
index 6f330ce..3a3f7f6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
 
 	if (skb_rx_queue_recorded(skb)) {
 		u16 index = skb_get_rx_queue(skb);
-		if (unlikely(index >= dev->num_rx_queues)) {
-			if (net_ratelimit()) {
-				pr_warning("%s received packet on queue "
-					"%u, but number of RX queues is %u\n",
-					dev->name, index, dev->num_rx_queues);
-			}
-			goto done;
-		}
+		if (WARN_ONCE(index >= dev->num_rx_queues,
+				KERN_WARNING "%s received packet on queue %u, "
+				"but number of RX queues is %u\n",
+				dev->name, index, dev->num_rx_queues))
+			index %= dev->num_rx_queues;
 		rxqueue = dev->_rx + index;
 	} else
 		rxqueue = dev->_rx;



2) bond/bridge should setup more queues, just in case.
   We probably need to be able to make things more dynamic,
   (propagate nbtxq between layers) but not for 2.6.35



diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5e12462..ce813dd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
 
 	rtnl_lock();
 
-	bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
-				bond_setup);
+	bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
+				   bond_setup, max(64, nr_cpu_ids));
 	if (!bond_dev) {
 		pr_err("%s: eek! can't alloc netdev!\n", name);
 		rtnl_unlock();

Re: RFS seems to have incompatiblities with bridged vlans

[Tom Herbert]

How about only checking against dev->num_rx_queues when that value is
greater than one.  Since bonding device is calling alloc_netdev, it is
not going to set the queue mapping, but dev->num_rx_queues will be one
in that case (this handles any intermediate driver that does do
multiple queues).

diff --git a/net/core/dev.c b/net/core/dev.c
index 6f330ce..30ab66d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2270,7 +2270,7 @@ static int get_rps_cpu(struct net_device *dev,
struct sk_buff *skb,
                u16 v16[2];
        } ports;

-       if (skb_rx_queue_recorded(skb)) {
+       if (skb_rx_queue_recorded(skb) && dev->num_rx_queues > 1) {
                u16 index = skb_get_rx_queue(skb);
                if (unlikely(index >= dev->num_rx_queues)) {
                        if (net_ratelimit()) {


On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>
>> There is always a possibility that the underlying device sets the
>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>> issue exists with bonding devices.  Maybe something like the following
>> is worth while? compile tested only,
>>
>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>
>> recv path reassigns skb->dev without sanity checking the
>> queue_mapping field.  This can result in the queue_mapping
>> field being set incorrectly if the new dev supports less
>> queues then the underlying device.
>>
>> This patch just resets the queue_mapping to 0 which should
>> resolve this issue?  Any thoughts?
>>
>> The same issue could happen on bonding devices as well.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>
>>   net/8021q/vlan_core.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index bd537fc..ad309f8 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>> vlan_group *grp,
>>       if (!skb->dev)
>>               goto drop;
>>
>> +     if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
>> +             skb_set_queue_mapping(skb, 0);
>> +
>>       return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>
>>   drop:
>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>> vlan_group *grp,
>>       if (!skb->dev)
>>               goto drop;
>>
>> +     if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
>> +             skb_set_queue_mapping(skb, 0);
>> +
>>       for (p = napi->gro_list; p; p = p->next) {
>>               NAPI_GRO_CB(p)->same_flow =
>>                       p->dev == skb->dev && !compare_ether_header(
>> --
>
> Only a workaround, added in hot path in a otherwise 'good' driver
> (multiqueue enabled and ready)
>
> eth0  -------> bond / bridge ---------> vlan.id
> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>
> X is capped to 1 because of bond/bridge, while bond has no "queue"
> (LLTX driver)
>
> Solutions :
>
> 1) queue_mapping could be silently tested in get_rps_cpu()...
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6f330ce..3a3f7f6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>
>        if (skb_rx_queue_recorded(skb)) {
>                u16 index = skb_get_rx_queue(skb);
> -               if (unlikely(index >= dev->num_rx_queues)) {
> -                       if (net_ratelimit()) {
> -                               pr_warning("%s received packet on queue "
> -                                       "%u, but number of RX queues is %u\n",
> -                                       dev->name, index, dev->num_rx_queues);
> -                       }
> -                       goto done;
> -               }
> +               if (WARN_ONCE(index >= dev->num_rx_queues,
> +                               KERN_WARNING "%s received packet on queue %u, "
> +                               "but number of RX queues is %u\n",
> +                               dev->name, index, dev->num_rx_queues))
> +                       index %= dev->num_rx_queues;
>                rxqueue = dev->_rx + index;
>        } else
>                rxqueue = dev->_rx;
>
>
>
> 2) bond/bridge should setup more queues, just in case.
>   We probably need to be able to make things more dynamic,
>   (propagate nbtxq between layers) but not for 2.6.35
>
>
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 5e12462..ce813dd 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>
>        rtnl_lock();
>
> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
> -                               bond_setup);
> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
> +                                  bond_setup, max(64, nr_cpu_ids));
>        if (!bond_dev) {
>                pr_err("%s: eek! can't alloc netdev!\n", name);
>                rtnl_unlock();
>
>
> --
> 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
>

Re: RFS seems to have incompatiblities with bridged vlans

[Tim Gardner]

[-- Attachment #1: Type: text/plain, Size: 6157 bytes --]

On 06/08/2010 05:00 PM, Tom Herbert wrote:
> How about only checking against dev->num_rx_queues when that value is
> greater than one.  Since bonding device is calling alloc_netdev, it is
> not going to set the queue mapping, but dev->num_rx_queues will be one
> in that case (this handles any intermediate driver that does do
> multiple queues).
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6f330ce..30ab66d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2270,7 +2270,7 @@ static int get_rps_cpu(struct net_device *dev,
> struct sk_buff *skb,
>                  u16 v16[2];
>          } ports;
>
> -       if (skb_rx_queue_recorded(skb)) {
> +       if (skb_rx_queue_recorded(skb)&&  dev->num_rx_queues>  1) {
>                  u16 index = skb_get_rx_queue(skb);
>                  if (unlikely(index>= dev->num_rx_queues)) {
>                          if (net_ratelimit()) {
>
>
> On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet<eric.dumazet@gmail.com>  wrote:
>> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>>
>>> There is always a possibility that the underlying device sets the
>>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>>> issue exists with bonding devices.  Maybe something like the following
>>> is worth while? compile tested only,
>>>
>>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>>
>>> recv path reassigns skb->dev without sanity checking the
>>> queue_mapping field.  This can result in the queue_mapping
>>> field being set incorrectly if the new dev supports less
>>> queues then the underlying device.
>>>
>>> This patch just resets the queue_mapping to 0 which should
>>> resolve this issue?  Any thoughts?
>>>
>>> The same issue could happen on bonding devices as well.
>>>
>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>> ---
>>>
>>>    net/8021q/vlan_core.c |    6 ++++++
>>>    1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>> index bd537fc..ad309f8 100644
>>> --- a/net/8021q/vlan_core.c
>>> +++ b/net/8021q/vlan_core.c
>>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>>> vlan_group *grp,
>>>        if (!skb->dev)
>>>                goto drop;
>>>
>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>> +             skb_set_queue_mapping(skb, 0);
>>> +
>>>        return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>>
>>>    drop:
>>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>>> vlan_group *grp,
>>>        if (!skb->dev)
>>>                goto drop;
>>>
>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>> +             skb_set_queue_mapping(skb, 0);
>>> +
>>>        for (p = napi->gro_list; p; p = p->next) {
>>>                NAPI_GRO_CB(p)->same_flow =
>>>                        p->dev == skb->dev&&  !compare_ether_header(
>>> --
>>
>> Only a workaround, added in hot path in a otherwise 'good' driver
>> (multiqueue enabled and ready)
>>
>> eth0  ------->  bond / bridge --------->  vlan.id
>> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>>
>> X is capped to 1 because of bond/bridge, while bond has no "queue"
>> (LLTX driver)
>>
>> Solutions :
>>
>> 1) queue_mapping could be silently tested in get_rps_cpu()...
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6f330ce..3a3f7f6 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>>
>>         if (skb_rx_queue_recorded(skb)) {
>>                 u16 index = skb_get_rx_queue(skb);
>> -               if (unlikely(index>= dev->num_rx_queues)) {
>> -                       if (net_ratelimit()) {
>> -                               pr_warning("%s received packet on queue "
>> -                                       "%u, but number of RX queues is %u\n",
>> -                                       dev->name, index, dev->num_rx_queues);
>> -                       }
>> -                       goto done;
>> -               }
>> +               if (WARN_ONCE(index>= dev->num_rx_queues,
>> +                               KERN_WARNING "%s received packet on queue %u, "
>> +                               "but number of RX queues is %u\n",
>> +                               dev->name, index, dev->num_rx_queues))
>> +                       index %= dev->num_rx_queues;
>>                 rxqueue = dev->_rx + index;
>>         } else
>>                 rxqueue = dev->_rx;
>>
>>
>>
>> 2) bond/bridge should setup more queues, just in case.
>>    We probably need to be able to make things more dynamic,
>>    (propagate nbtxq between layers) but not for 2.6.35
>>
>>
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 5e12462..ce813dd 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>>
>>         rtnl_lock();
>>
>> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
>> -                               bond_setup);
>> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
>> +                                  bond_setup, max(64, nr_cpu_ids));
>>         if (!bond_dev) {
>>                 pr_err("%s: eek! can't alloc netdev!\n", name);
>>                 rtnl_unlock();
>>
>>
>> --
>> 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
>>
> --
> 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

Huh, so you guys are looking at the same issue (only my issue is RPS). 
See http://marc.info/?l=linux-netdev&m=127603240621028&w=2. I'm in favor 
of dropping the warning when no queues have been allocated.

How about this (see attached).

rtg

-- 
Tim Gardner tim.gardner@canonical.com

[-- Attachment #2: 0001-net-Print-num_rx_queues-warning-only-when-there-are-.patch --]
[-- Type: text/x-patch, Size: 1152 bytes --]

>From 391b0140b5c7e410d55258a8a2541bddf84d8d0e Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Tue, 8 Jun 2010 17:51:27 -0600
Subject: [PATCH] net: Print num_rx_queues warning only when there are allocated queues

Most users of skb_record_rx_queue() do not use alloc_netdev_mq() for
network device initialization, so don't print a warning about num_rx_queues
overflow in get_rps_cpu() unless they have actually been allocated.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 net/core/dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d03470f..0852608 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2253,7 +2253,7 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
 	if (skb_rx_queue_recorded(skb)) {
 		u16 index = skb_get_rx_queue(skb);
 		if (unlikely(index >= dev->num_rx_queues)) {
-			if (net_ratelimit()) {
+			if (dev->num_rx_queues > 1 && net_ratelimit()) {
 				pr_warning("%s received packet on queue "
 					"%u, but number of RX queues is %u\n",
 					dev->name, index, dev->num_rx_queues);
-- 
1.7.0.4

Re: RFS seems to have incompatiblities with bridged vlans

[John Fastabend]

Tim Gardner wrote:
> On 06/08/2010 05:00 PM, Tom Herbert wrote:
>> How about only checking against dev->num_rx_queues when that value is
>> greater than one.  Since bonding device is calling alloc_netdev, it is
>> not going to set the queue mapping, but dev->num_rx_queues will be one
>> in that case (this handles any intermediate driver that does do
>> multiple queues).
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6f330ce..30ab66d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2270,7 +2270,7 @@ static int get_rps_cpu(struct net_device *dev,
>> struct sk_buff *skb,
>>                  u16 v16[2];
>>          } ports;
>>
>> -       if (skb_rx_queue_recorded(skb)) {
>> +       if (skb_rx_queue_recorded(skb)&&  dev->num_rx_queues>  1) {
>>                  u16 index = skb_get_rx_queue(skb);
>>                  if (unlikely(index>= dev->num_rx_queues)) {
>>                          if (net_ratelimit()) {
>>


Problem with this is it doesn't address mis-aligned num_rx_queues.  For example 
with the bonding driver defaulting to 16 queues now. We could end up with a base 
driver with 16+ queues and a bond with 16.  Then we have the same issue again.

  eth0  -------> bond / bridge ---------> vlan.id
(nbrxq=64)      (nbrxq=16)              (nbrxq=X)

>>
>> On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet<eric.dumazet@gmail.com>  wrote:
>>> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>>>
>>>> There is always a possibility that the underlying device sets the
>>>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>>>> issue exists with bonding devices.  Maybe something like the following
>>>> is worth while? compile tested only,
>>>>
>>>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>>>
>>>> recv path reassigns skb->dev without sanity checking the
>>>> queue_mapping field.  This can result in the queue_mapping
>>>> field being set incorrectly if the new dev supports less
>>>> queues then the underlying device.
>>>>
>>>> This patch just resets the queue_mapping to 0 which should
>>>> resolve this issue?  Any thoughts?
>>>>
>>>> The same issue could happen on bonding devices as well.
>>>>
>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>>> ---
>>>>
>>>>    net/8021q/vlan_core.c |    6 ++++++
>>>>    1 files changed, 6 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>>> index bd537fc..ad309f8 100644
>>>> --- a/net/8021q/vlan_core.c
>>>> +++ b/net/8021q/vlan_core.c
>>>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>>>> vlan_group *grp,
>>>>        if (!skb->dev)
>>>>                goto drop;
>>>>
>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>> +             skb_set_queue_mapping(skb, 0);
>>>> +
>>>>        return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>>>
>>>>    drop:
>>>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>>>> vlan_group *grp,
>>>>        if (!skb->dev)
>>>>                goto drop;
>>>>
>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>> +             skb_set_queue_mapping(skb, 0);
>>>> +
>>>>        for (p = napi->gro_list; p; p = p->next) {
>>>>                NAPI_GRO_CB(p)->same_flow =
>>>>                        p->dev == skb->dev&&  !compare_ether_header(
>>>> --
>>> Only a workaround, added in hot path in a otherwise 'good' driver
>>> (multiqueue enabled and ready)

Agreed thanks!

>>>
>>> eth0  ------->  bond / bridge --------->  vlan.id
>>> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>>>
>>> X is capped to 1 because of bond/bridge, while bond has no "queue"
>>> (LLTX driver)
>>>
>>> Solutions :
>>>
>>> 1) queue_mapping could be silently tested in get_rps_cpu()...
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 6f330ce..3a3f7f6 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>>>
>>>         if (skb_rx_queue_recorded(skb)) {
>>>                 u16 index = skb_get_rx_queue(skb);
>>> -               if (unlikely(index>= dev->num_rx_queues)) {
>>> -                       if (net_ratelimit()) {
>>> -                               pr_warning("%s received packet on queue "
>>> -                                       "%u, but number of RX queues is %u\n",
>>> -                                       dev->name, index, dev->num_rx_queues);
>>> -                       }
>>> -                       goto done;
>>> -               }
>>> +               if (WARN_ONCE(index>= dev->num_rx_queues,
>>> +                               KERN_WARNING "%s received packet on queue %u, "
>>> +                               "but number of RX queues is %u\n",
>>> +                               dev->name, index, dev->num_rx_queues))
>>> +                       index %= dev->num_rx_queues;
>>>                 rxqueue = dev->_rx + index;
>>>         } else
>>>                 rxqueue = dev->_rx;
>>>
>>>

Looks good to me.

>>>
>>> 2) bond/bridge should setup more queues, just in case.
>>>    We probably need to be able to make things more dynamic,
>>>    (propagate nbtxq between layers) but not for 2.6.35
>>>
>>>

The bonding driver is already multiq per Andy Gospodarek's patch commit bb1d912, 
but unless the bond and bridge devices use the max num_rx_queues of there 
underlying devices this could still go wrong.

The bonding driver would possibly need to increase num_rx_queues and 
num_tx_queues when a device is enslaved or be set to some maximum at init for 
this to work right.

>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 5e12462..ce813dd 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>>>
>>>         rtnl_lock();
>>>
>>> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
>>> -                               bond_setup);
>>> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
>>> +                                  bond_setup, max(64, nr_cpu_ids));
>>>         if (!bond_dev) {
>>>                 pr_err("%s: eek! can't alloc netdev!\n", name);
>>>                 rtnl_unlock();
>>>
>>>
>>> --
>>> 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
>>>
>> --
>> 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
> 
> Huh, so you guys are looking at the same issue (only my issue is RPS). 
> See http://marc.info/?l=linux-netdev&m=127603240621028&w=2. I'm in favor 
> of dropping the warning when no queues have been allocated.
> 
> How about this (see attached).

Prefer Eric's patch see first comment.

Thanks,
John

> 
> rtg
> 
> 

Re: RFS seems to have incompatiblities with bridged vlans

[Tom Herbert]

>
> Problem with this is it doesn't address mis-aligned num_rx_queues.  For
> example with the bonding driver defaulting to 16 queues now. We could end up
> with a base driver with 16+ queues and a bond with 16.  Then we have the
> same issue again.
>
>  eth0  -------> bond / bridge ---------> vlan.id
> (nbrxq=64)      (nbrxq=16)              (nbrxq=X)
>

I don't think the intent is to solve this alignment problem here.  If
a driver allocates multiple queues, it should set the queue mapping
accordingly.  If this isn't the case, the meaning of queue mapping is
ambiguous (which driver is it relative to?), and using the value for
rps_cpus is probably not going to work well-- hacking the
queue-mapping to be a legal value in get_rps_cpus doesn't seem like
the right answer.

>>>
>>> On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet<eric.dumazet@gmail.com>
>>>  wrote:
>>>>
>>>> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>>>>
>>>>> There is always a possibility that the underlying device sets the
>>>>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>>>>> issue exists with bonding devices.  Maybe something like the following
>>>>> is worth while? compile tested only,
>>>>>
>>>>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>>>>
>>>>> recv path reassigns skb->dev without sanity checking the
>>>>> queue_mapping field.  This can result in the queue_mapping
>>>>> field being set incorrectly if the new dev supports less
>>>>> queues then the underlying device.
>>>>>
>>>>> This patch just resets the queue_mapping to 0 which should
>>>>> resolve this issue?  Any thoughts?
>>>>>
>>>>> The same issue could happen on bonding devices as well.
>>>>>
>>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>>>> ---
>>>>>
>>>>>   net/8021q/vlan_core.c |    6 ++++++
>>>>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>>>> index bd537fc..ad309f8 100644
>>>>> --- a/net/8021q/vlan_core.c
>>>>> +++ b/net/8021q/vlan_core.c
>>>>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>>>>> vlan_group *grp,
>>>>>       if (!skb->dev)
>>>>>               goto drop;
>>>>>
>>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>>> +             skb_set_queue_mapping(skb, 0);
>>>>> +
>>>>>       return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>>>>
>>>>>   drop:
>>>>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>>>>> vlan_group *grp,
>>>>>       if (!skb->dev)
>>>>>               goto drop;
>>>>>
>>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>>> +             skb_set_queue_mapping(skb, 0);
>>>>> +
>>>>>       for (p = napi->gro_list; p; p = p->next) {
>>>>>               NAPI_GRO_CB(p)->same_flow =
>>>>>                       p->dev == skb->dev&&  !compare_ether_header(
>>>>> --
>>>>
>>>> Only a workaround, added in hot path in a otherwise 'good' driver
>>>> (multiqueue enabled and ready)
>
> Agreed thanks!
>
>>>>
>>>> eth0  ------->  bond / bridge --------->  vlan.id
>>>> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>>>>
>>>> X is capped to 1 because of bond/bridge, while bond has no "queue"
>>>> (LLTX driver)
>>>>
>>>> Solutions :
>>>>
>>>> 1) queue_mapping could be silently tested in get_rps_cpu()...
>>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index 6f330ce..3a3f7f6 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev,
>>>> struct sk_buff *skb,
>>>>
>>>>        if (skb_rx_queue_recorded(skb)) {
>>>>                u16 index = skb_get_rx_queue(skb);
>>>> -               if (unlikely(index>= dev->num_rx_queues)) {
>>>> -                       if (net_ratelimit()) {
>>>> -                               pr_warning("%s received packet on queue
>>>> "
>>>> -                                       "%u, but number of RX queues is
>>>> %u\n",
>>>> -                                       dev->name, index,
>>>> dev->num_rx_queues);
>>>> -                       }
>>>> -                       goto done;
>>>> -               }
>>>> +               if (WARN_ONCE(index>= dev->num_rx_queues,
>>>> +                               KERN_WARNING "%s received packet on
>>>> queue %u, "
>>>> +                               "but number of RX queues is %u\n",
>>>> +                               dev->name, index, dev->num_rx_queues))
>>>> +                       index %= dev->num_rx_queues;
>>>>                rxqueue = dev->_rx + index;
>>>>        } else
>>>>                rxqueue = dev->_rx;
>>>>
>>>>
>
> Looks good to me.
>
>>>>
>>>> 2) bond/bridge should setup more queues, just in case.
>>>>   We probably need to be able to make things more dynamic,
>>>>   (propagate nbtxq between layers) but not for 2.6.35
>>>>
>>>>
>
> The bonding driver is already multiq per Andy Gospodarek's patch commit
> bb1d912, but unless the bond and bridge devices use the max num_rx_queues of
> there underlying devices this could still go wrong.
>
> The bonding driver would possibly need to increase num_rx_queues and
> num_tx_queues when a device is enslaved or be set to some maximum at init
> for this to work right.
>
>>>>
>>>> diff --git a/drivers/net/bonding/bond_main.c
>>>> b/drivers/net/bonding/bond_main.c
>>>> index 5e12462..ce813dd 100644
>>>> --- a/drivers/net/bonding/bond_main.c
>>>> +++ b/drivers/net/bonding/bond_main.c
>>>> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>>>>
>>>>        rtnl_lock();
>>>>
>>>> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name :
>>>> "",
>>>> -                               bond_setup);
>>>> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name :
>>>> "",
>>>> +                                  bond_setup, max(64, nr_cpu_ids));
>>>>        if (!bond_dev) {
>>>>                pr_err("%s: eek! can't alloc netdev!\n", name);
>>>>                rtnl_unlock();
>>>>
>>>>
>>>> --
>>>> 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
>>>>
>>> --
>>> 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
>>
>> Huh, so you guys are looking at the same issue (only my issue is RPS). See
>> http://marc.info/?l=linux-netdev&m=127603240621028&w=2. I'm in favor of
>> dropping the warning when no queues have been allocated.
>>
>> How about this (see attached).
>
> Prefer Eric's patch see first comment.
>
> Thanks,
> John
>
>>
>> rtg
>>
>>
>
>

Re: RFS seems to have incompatiblities with bridged vlans

[Eric Dumazet]

Le mardi 08 juin 2010 à 18:08 -0700, John Fastabend a écrit :


> 
> The bonding driver is already multiq per Andy Gospodarek's patch commit bb1d912, 
> but unless the bond and bridge devices use the max num_rx_queues of there 
> underlying devices this could still go wrong.
> 

This patch is in net-next-2.6, it is not scheduled for 2.6.35

We need some fix for 2.6.35

> The bonding driver would possibly need to increase num_rx_queues and 
> num_tx_queues when a device is enslaved or be set to some maximum at init for 
> this to work right.

Thats a bit complex and definitly not 2.6.35 material.