[net-next PATCH] net: allow vlan traffic to be received under bond

[net-next PATCH] net: allow vlan traffic to be received under bond

[John Fastabend]

The following configuration used to work as I expected. At least
we could use the fcoe interfaces to do MPIO and the bond0 iface
to do load balancing or failover.

       ---eth2.228-fcoe
       |
eth2 -----|
          |
          |---- bond0
          |
eth3 -----|
       |
       ---eth3.228-fcoe

This worked because of a change we added to allow inactive slaves
to rx 'exact' matches. This functionality was kept intact with the
rx_handler mechanism. However now the vlan interface attached to the
active slave never receives traffic because the bonding rx_handler
updates the skb->dev and goto's another_round. Previously, the
vlan_do_receive() logic was called before the bonding rx_handler.

Now by the time vlan_do_receive calls vlan_find_dev() the
skb->dev is set to bond0 and it is clear no vlan is attached
to this iface. The vlan lookup fails.

This patch moves the VLAN check above the rx_handler. A VLAN
tagged frame is now routed to the eth2.228-fcoe iface in the
above schematic. Untagged frames continue to the bond0 as
normal. This case also remains intact,

eth2 --> bond0 --> vlan.228

Here the skb is VLAN tagged but the vlan lookup fails on eth2
causing the bonding rx_handler to be called. On the second
pass the vlan lookup is on the bond0 iface and completes as
expected.

Putting a VLAN.228 on both the bond0 and eth2 device will
result in eth2.228 receiving the skb. I don't think this is
completely unexpected and was the result prior to the rx_handler
result.

Note, the same setup is also used for other storage traffic that
MPIO is used with eg. iSCSI and similar setups can be contrived
without storage protocols.

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

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

diff --git a/net/core/dev.c b/net/core/dev.c
index 70ecb86..8b6118a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3231,6 +3231,17 @@ another_round:
 ncls:
 #endif
 
+	if (vlan_tx_tag_present(skb)) {
+		if (pt_prev) {
+			ret = deliver_skb(skb, pt_prev, orig_dev);
+			pt_prev = NULL;
+		}
+		if (vlan_do_receive(&skb))
+			goto another_round;
+		else if (unlikely(!skb))
+			goto out;
+	}
+
 	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (rx_handler) {
 		if (pt_prev) {
@@ -3251,17 +3262,6 @@ ncls:
 		}
 	}
 
-	if (vlan_tx_tag_present(skb)) {
-		if (pt_prev) {
-			ret = deliver_skb(skb, pt_prev, orig_dev);
-			pt_prev = NULL;
-		}
-		if (vlan_do_receive(&skb))
-			goto another_round;
-		else if (unlikely(!skb))
-			goto out;
-	}
-
 	/* deliver only exact match when indicated */
 	null_or_dev = deliver_exact ? skb->dev : NULL;
 

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Jiri Pirko]

Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
>The following configuration used to work as I expected. At least
>we could use the fcoe interfaces to do MPIO and the bond0 iface
>to do load balancing or failover.
>
>       ---eth2.228-fcoe
>       |
>eth2 -----|
>          |
>          |---- bond0
>          |
>eth3 -----|
>       |
>       ---eth3.228-fcoe
>
>This worked because of a change we added to allow inactive slaves
>to rx 'exact' matches. This functionality was kept intact with the
>rx_handler mechanism. However now the vlan interface attached to the
>active slave never receives traffic because the bonding rx_handler
>updates the skb->dev and goto's another_round. Previously, the
>vlan_do_receive() logic was called before the bonding rx_handler.
>
>Now by the time vlan_do_receive calls vlan_find_dev() the
>skb->dev is set to bond0 and it is clear no vlan is attached
>to this iface. The vlan lookup fails.
>
>This patch moves the VLAN check above the rx_handler. A VLAN
>tagged frame is now routed to the eth2.228-fcoe iface in the
>above schematic. Untagged frames continue to the bond0 as
>normal. This case also remains intact,
>
>eth2 --> bond0 --> vlan.228
>
>Here the skb is VLAN tagged but the vlan lookup fails on eth2
>causing the bonding rx_handler to be called. On the second
>pass the vlan lookup is on the bond0 iface and completes as
>expected.
>
>Putting a VLAN.228 on both the bond0 and eth2 device will
>result in eth2.228 receiving the skb. I don't think this is
>completely unexpected and was the result prior to the rx_handler
>result.
>
>Note, the same setup is also used for other storage traffic that
>MPIO is used with eg. iSCSI and similar setups can be contrived
>without storage protocols.
>
>Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>---
>
> net/core/dev.c |   22 +++++++++++-----------
> 1 files changed, 11 insertions(+), 11 deletions(-)
>
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 70ecb86..8b6118a 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -3231,6 +3231,17 @@ another_round:
> ncls:
> #endif
> 
>+	if (vlan_tx_tag_present(skb)) {
>+		if (pt_prev) {
>+			ret = deliver_skb(skb, pt_prev, orig_dev);
>+			pt_prev = NULL;
>+		}
>+		if (vlan_do_receive(&skb))
>+			goto another_round;
>+		else if (unlikely(!skb))
>+			goto out;
>+	}
>+
> 	rx_handler = rcu_dereference(skb->dev->rx_handler);
> 	if (rx_handler) {
> 		if (pt_prev) {
>@@ -3251,17 +3262,6 @@ ncls:
> 		}
> 	}
> 
>-	if (vlan_tx_tag_present(skb)) {
>-		if (pt_prev) {
>-			ret = deliver_skb(skb, pt_prev, orig_dev);
>-			pt_prev = NULL;
>-		}
>-		if (vlan_do_receive(&skb))
>-			goto another_round;
>-		else if (unlikely(!skb))
>-			goto out;
>-	}
>-
> 	/* deliver only exact match when indicated */
> 	null_or_dev = deliver_exact ? skb->dev : NULL;
> 
>

Hmm, I must look at this again tomorrow but I have strong feeling this
will break some some scenario including vlan-bridge-macvlan.

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[John Fastabend]

On 10/10/2011 3:37 PM, Jiri Pirko wrote:
> Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
>> The following configuration used to work as I expected. At least
>> we could use the fcoe interfaces to do MPIO and the bond0 iface
>> to do load balancing or failover.
>>
>>       ---eth2.228-fcoe
>>       |
>> eth2 -----|
>>          |
>>          |---- bond0
>>          |
>> eth3 -----|
>>       |
>>       ---eth3.228-fcoe
>>
>> This worked because of a change we added to allow inactive slaves
>> to rx 'exact' matches. This functionality was kept intact with the
>> rx_handler mechanism. However now the vlan interface attached to the
>> active slave never receives traffic because the bonding rx_handler
>> updates the skb->dev and goto's another_round. Previously, the
>> vlan_do_receive() logic was called before the bonding rx_handler.
>>
>> Now by the time vlan_do_receive calls vlan_find_dev() the
>> skb->dev is set to bond0 and it is clear no vlan is attached
>> to this iface. The vlan lookup fails.
>>
>> This patch moves the VLAN check above the rx_handler. A VLAN
>> tagged frame is now routed to the eth2.228-fcoe iface in the
>> above schematic. Untagged frames continue to the bond0 as
>> normal. This case also remains intact,
>>
>> eth2 --> bond0 --> vlan.228
>>
>> Here the skb is VLAN tagged but the vlan lookup fails on eth2
>> causing the bonding rx_handler to be called. On the second
>> pass the vlan lookup is on the bond0 iface and completes as
>> expected.
>>
>> Putting a VLAN.228 on both the bond0 and eth2 device will
>> result in eth2.228 receiving the skb. I don't think this is
>> completely unexpected and was the result prior to the rx_handler
>> result.
>>
>> Note, the same setup is also used for other storage traffic that
>> MPIO is used with eg. iSCSI and similar setups can be contrived
>> without storage protocols.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>
>> net/core/dev.c |   22 +++++++++++-----------
>> 1 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 70ecb86..8b6118a 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3231,6 +3231,17 @@ another_round:
>> ncls:
>> #endif
>>
>> +	if (vlan_tx_tag_present(skb)) {
>> +		if (pt_prev) {
>> +			ret = deliver_skb(skb, pt_prev, orig_dev);
>> +			pt_prev = NULL;
>> +		}
>> +		if (vlan_do_receive(&skb))
>> +			goto another_round;
>> +		else if (unlikely(!skb))
>> +			goto out;
>> +	}
>> +
>> 	rx_handler = rcu_dereference(skb->dev->rx_handler);
>> 	if (rx_handler) {
>> 		if (pt_prev) {
>> @@ -3251,17 +3262,6 @@ ncls:
>> 		}
>> 	}
>>
>> -	if (vlan_tx_tag_present(skb)) {
>> -		if (pt_prev) {
>> -			ret = deliver_skb(skb, pt_prev, orig_dev);
>> -			pt_prev = NULL;
>> -		}
>> -		if (vlan_do_receive(&skb))
>> -			goto another_round;
>> -		else if (unlikely(!skb))
>> -			goto out;
>> -	}
>> -
>> 	/* deliver only exact match when indicated */
>> 	null_or_dev = deliver_exact ? skb->dev : NULL;
>>
>>
> 
> Hmm, I must look at this again tomorrow but I have strong feeling this
> will break some some scenario including vlan-bridge-macvlan.

Yes please review... I tested cases with vlan, bridge, and macvlan
components and believe this works unless I missed something.

Maybe Jesse, can comment though on why this commit that moved (and
cleaned up) the vlan tag handling put the vlan_do_receive below the
rx_handler rather than above it. Was this intended to fix something?

commit 3701e51382a026cba10c60b03efabe534fba4ca4
Author: Jesse Gross <jesse@nicira.com>
Date:   Wed Oct 20 13:56:06 2010 +0000

    vlan: Centralize handling of hardware acceleration.


Thanks,
John.

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Jesse Gross]

On Mon, Oct 10, 2011 at 7:07 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> On 10/10/2011 3:37 PM, Jiri Pirko wrote:
>> Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
>>> The following configuration used to work as I expected. At least
>>> we could use the fcoe interfaces to do MPIO and the bond0 iface
>>> to do load balancing or failover.
>>>
>>>       ---eth2.228-fcoe
>>>       |
>>> eth2 -----|
>>>          |
>>>          |---- bond0
>>>          |
>>> eth3 -----|
>>>       |
>>>       ---eth3.228-fcoe
>>>
>>> This worked because of a change we added to allow inactive slaves
>>> to rx 'exact' matches. This functionality was kept intact with the
>>> rx_handler mechanism. However now the vlan interface attached to the
>>> active slave never receives traffic because the bonding rx_handler
>>> updates the skb->dev and goto's another_round. Previously, the
>>> vlan_do_receive() logic was called before the bonding rx_handler.
>>>
>>> Now by the time vlan_do_receive calls vlan_find_dev() the
>>> skb->dev is set to bond0 and it is clear no vlan is attached
>>> to this iface. The vlan lookup fails.
>>>
>>> This patch moves the VLAN check above the rx_handler. A VLAN
>>> tagged frame is now routed to the eth2.228-fcoe iface in the
>>> above schematic. Untagged frames continue to the bond0 as
>>> normal. This case also remains intact,
>>>
>>> eth2 --> bond0 --> vlan.228
>>>
>>> Here the skb is VLAN tagged but the vlan lookup fails on eth2
>>> causing the bonding rx_handler to be called. On the second
>>> pass the vlan lookup is on the bond0 iface and completes as
>>> expected.
>>>
>>> Putting a VLAN.228 on both the bond0 and eth2 device will
>>> result in eth2.228 receiving the skb. I don't think this is
>>> completely unexpected and was the result prior to the rx_handler
>>> result.
>>>
>>> Note, the same setup is also used for other storage traffic that
>>> MPIO is used with eg. iSCSI and similar setups can be contrived
>>> without storage protocols.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>> ---
>>>
>>> net/core/dev.c |   22 +++++++++++-----------
>>> 1 files changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 70ecb86..8b6118a 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -3231,6 +3231,17 @@ another_round:
>>> ncls:
>>> #endif
>>>
>>> +    if (vlan_tx_tag_present(skb)) {
>>> +            if (pt_prev) {
>>> +                    ret = deliver_skb(skb, pt_prev, orig_dev);
>>> +                    pt_prev = NULL;
>>> +            }
>>> +            if (vlan_do_receive(&skb))
>>> +                    goto another_round;
>>> +            else if (unlikely(!skb))
>>> +                    goto out;
>>> +    }
>>> +
>>>      rx_handler = rcu_dereference(skb->dev->rx_handler);
>>>      if (rx_handler) {
>>>              if (pt_prev) {
>>> @@ -3251,17 +3262,6 @@ ncls:
>>>              }
>>>      }
>>>
>>> -    if (vlan_tx_tag_present(skb)) {
>>> -            if (pt_prev) {
>>> -                    ret = deliver_skb(skb, pt_prev, orig_dev);
>>> -                    pt_prev = NULL;
>>> -            }
>>> -            if (vlan_do_receive(&skb))
>>> -                    goto another_round;
>>> -            else if (unlikely(!skb))
>>> -                    goto out;
>>> -    }
>>> -
>>>      /* deliver only exact match when indicated */
>>>      null_or_dev = deliver_exact ? skb->dev : NULL;
>>>
>>>
>>
>> Hmm, I must look at this again tomorrow but I have strong feeling this
>> will break some some scenario including vlan-bridge-macvlan.
>
> Yes please review... I tested cases with vlan, bridge, and macvlan
> components and believe this works unless I missed something.
>
> Maybe Jesse, can comment though on why this commit that moved (and
> cleaned up) the vlan tag handling put the vlan_do_receive below the
> rx_handler rather than above it. Was this intended to fix something?

The original reason was to ensure packets received from NICs that do
stripping behaved the same as those that don't.  At the time, the
packets with inline vlan tags were handled by the same code that
handled upper layer protocols so it was important that code for vlan
stripped tags be immediately before that.  Otherwise, packets might be
taken either by the bridge hook or vlan code depending the the type of
device.

However, that's no longer an issue because we now emulate vlan
acceleration by untagging packets at the beginning of
__netif_receive_skb(), so the code path will always be the same.
Furthermore, based on feedback received since that patch it seems
pretty clear that people prefer the behavior where vlan devices take
traffic first, so now that we can have both that and consistent
behavior it seems to be the way to go.

This looks correct to me:
Acked-by: Jesse Gross <jesse@nicira.com>

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Hans Schillstrom]

Hello
On Tuesday 11 October 2011 04:43:03 Jesse Gross wrote:
> On Mon, Oct 10, 2011 at 7:07 PM, John Fastabend
> <john.r.fastabend@intel.com> wrote:
> > On 10/10/2011 3:37 PM, Jiri Pirko wrote:
> >> Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
> >>> The following configuration used to work as I expected. At least
> >>> we could use the fcoe interfaces to do MPIO and the bond0 iface
> >>> to do load balancing or failover.
> >>>
> >>>       ---eth2.228-fcoe
> >>>       |
> >>> eth2 -----|
> >>>          |
> >>>          |---- bond0
> >>>          |
> >>> eth3 -----|
> >>>       |
> >>>       ---eth3.228-fcoe
> >>>
> >>> This worked because of a change we added to allow inactive slaves
> >>> to rx 'exact' matches. This functionality was kept intact with the
> >>> rx_handler mechanism. However now the vlan interface attached to the
> >>> active slave never receives traffic because the bonding rx_handler
> >>> updates the skb->dev and goto's another_round. Previously, the
> >>> vlan_do_receive() logic was called before the bonding rx_handler.
> >>>
> >>> Now by the time vlan_do_receive calls vlan_find_dev() the
> >>> skb->dev is set to bond0 and it is clear no vlan is attached
> >>> to this iface. The vlan lookup fails.
> >>>
> >>> This patch moves the VLAN check above the rx_handler. A VLAN
> >>> tagged frame is now routed to the eth2.228-fcoe iface in the
> >>> above schematic. Untagged frames continue to the bond0 as
> >>> normal. This case also remains intact,
> >>>
> >>> eth2 --> bond0 --> vlan.228
> >>>
> >>> Here the skb is VLAN tagged but the vlan lookup fails on eth2
> >>> causing the bonding rx_handler to be called. On the second
> >>> pass the vlan lookup is on the bond0 iface and completes as
> >>> expected.
> >>>
> >>> Putting a VLAN.228 on both the bond0 and eth2 device will
> >>> result in eth2.228 receiving the skb. I don't think this is
> >>> completely unexpected and was the result prior to the rx_handler
> >>> result.

I think this OK, but I do have a question
if bond0 is in Active/Backup mode, eth2 and eth3 got the same MAC.addr,
what about the VLAN:s ?
(or is just one of thme working ??)

> >>>
> >>> Note, the same setup is also used for other storage traffic that
> >>> MPIO is used with eg. iSCSI and similar setups can be contrived
> >>> without storage protocols.
> >>>
> >>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> >>> ---
> >>>
> >>> net/core/dev.c |   22 +++++++++++-----------
> >>> 1 files changed, 11 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>> index 70ecb86..8b6118a 100644
> >>> --- a/net/core/dev.c
> >>> +++ b/net/core/dev.c
> >>> @@ -3231,6 +3231,17 @@ another_round:
> >>> ncls:
> >>> #endif
> >>>
> >>> +    if (vlan_tx_tag_present(skb)) {
> >>> +            if (pt_prev) {
> >>> +                    ret = deliver_skb(skb, pt_prev, orig_dev);
> >>> +                    pt_prev = NULL;
> >>> +            }
> >>> +            if (vlan_do_receive(&skb))
> >>> +                    goto another_round;
> >>> +            else if (unlikely(!skb))
> >>> +                    goto out;
> >>> +    }
> >>> +
> >>>      rx_handler = rcu_dereference(skb->dev->rx_handler);
> >>>      if (rx_handler) {
> >>>              if (pt_prev) {
> >>> @@ -3251,17 +3262,6 @@ ncls:
> >>>              }
> >>>      }
> >>>
> >>> -    if (vlan_tx_tag_present(skb)) {
> >>> -            if (pt_prev) {
> >>> -                    ret = deliver_skb(skb, pt_prev, orig_dev);
> >>> -                    pt_prev = NULL;
> >>> -            }
> >>> -            if (vlan_do_receive(&skb))
> >>> -                    goto another_round;
> >>> -            else if (unlikely(!skb))
> >>> -                    goto out;
> >>> -    }
> >>> -
> >>>      /* deliver only exact match when indicated */
> >>>      null_or_dev = deliver_exact ? skb->dev : NULL;
> >>>
> >>>
> >>
> >> Hmm, I must look at this again tomorrow but I have strong feeling this
> >> will break some some scenario including vlan-bridge-macvlan.
> >
> > Yes please review... I tested cases with vlan, bridge, and macvlan
> > components and believe this works unless I missed something.
> >
> > Maybe Jesse, can comment though on why this commit that moved (and
> > cleaned up) the vlan tag handling put the vlan_do_receive below the
> > rx_handler rather than above it. Was this intended to fix something?
> 
> The original reason was to ensure packets received from NICs that do
> stripping behaved the same as those that don't.  At the time, the
> packets with inline vlan tags were handled by the same code that
> handled upper layer protocols so it was important that code for vlan
> stripped tags be immediately before that.  Otherwise, packets might be
> taken either by the bridge hook or vlan code depending the the type of
> device.
> 
> However, that's no longer an issue because we now emulate vlan
> acceleration by untagging packets at the beginning of
> __netif_receive_skb(), so the code path will always be the same.
> Furthermore, based on feedback received since that patch it seems
> pretty clear that people prefer the behavior where vlan devices take
> traffic first, so now that we can have both that and consistent
> behavior it seems to be the way to go.
> 
> This looks correct to me:
> Acked-by: Jesse Gross <jesse@nicira.com>
> --
> 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
> 

-- 
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[John Fastabend]

On 10/11/2011 4:08 AM, Hans Schillstrom wrote:
> Hello
> On Tuesday 11 October 2011 04:43:03 Jesse Gross wrote:
>> On Mon, Oct 10, 2011 at 7:07 PM, John Fastabend
>> <john.r.fastabend@intel.com> wrote:
>>> On 10/10/2011 3:37 PM, Jiri Pirko wrote:
>>>> Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
>>>>> The following configuration used to work as I expected. At least
>>>>> we could use the fcoe interfaces to do MPIO and the bond0 iface
>>>>> to do load balancing or failover.
>>>>>
>>>>>       ---eth2.228-fcoe
>>>>>       |
>>>>> eth2 -----|
>>>>>          |
>>>>>          |---- bond0
>>>>>          |
>>>>> eth3 -----|
>>>>>       |
>>>>>       ---eth3.228-fcoe
>>>>>
>>>>> This worked because of a change we added to allow inactive slaves
>>>>> to rx 'exact' matches. This functionality was kept intact with the
>>>>> rx_handler mechanism. However now the vlan interface attached to the
>>>>> active slave never receives traffic because the bonding rx_handler
>>>>> updates the skb->dev and goto's another_round. Previously, the
>>>>> vlan_do_receive() logic was called before the bonding rx_handler.
>>>>>
>>>>> Now by the time vlan_do_receive calls vlan_find_dev() the
>>>>> skb->dev is set to bond0 and it is clear no vlan is attached
>>>>> to this iface. The vlan lookup fails.
>>>>>
>>>>> This patch moves the VLAN check above the rx_handler. A VLAN
>>>>> tagged frame is now routed to the eth2.228-fcoe iface in the
>>>>> above schematic. Untagged frames continue to the bond0 as
>>>>> normal. This case also remains intact,
>>>>>
>>>>> eth2 --> bond0 --> vlan.228
>>>>>
>>>>> Here the skb is VLAN tagged but the vlan lookup fails on eth2
>>>>> causing the bonding rx_handler to be called. On the second
>>>>> pass the vlan lookup is on the bond0 iface and completes as
>>>>> expected.
>>>>>
>>>>> Putting a VLAN.228 on both the bond0 and eth2 device will
>>>>> result in eth2.228 receiving the skb. I don't think this is
>>>>> completely unexpected and was the result prior to the rx_handler
>>>>> result.
> 
> I think this OK, but I do have a question
> if bond0 is in Active/Backup mode, eth2 and eth3 got the same MAC.addr,
> what about the VLAN:s ?
> (or is just one of thme working ??)
> 

The VLAN MAC address will not be managed by the bond. In the
storage case a SAN mac may be used (NETDEV_HW_ADDR_T_SAN).
Otherwise the MAC can be managed normally.

Both VLANs will receive frames but in some modes only to packet
handlers that have exact matches. See bond_should_deliver_exact_match().

.John.

RE: [net-next PATCH] net: allow vlan traffic to be received under bond

[Hans Schillström]

>On 10/11/2011 4:08 AM, Hans Schillstrom wrote:
>> Hello
>> On Tuesday 11 October 2011 04:43:03 Jesse Gross wrote:
>>> On Mon, Oct 10, 2011 at 7:07 PM, John Fastabend
>>> <john.r.fastabend@intel.com> wrote:
>>>> On 10/10/2011 3:37 PM, Jiri Pirko wrote:
>>>>> Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
>>>>>> The following configuration used to work as I expected. At least
>>>>>> we could use the fcoe interfaces to do MPIO and the bond0 iface
>>>>>> to do load balancing or failover.
>>>>>>
>>>>>>       ---eth2.228-fcoe
>>>>>>       |
>>>>>> eth2 -----|
>>>>>>          |
>>>>>>          |---- bond0
>>>>>>          |
>>>>>> eth3 -----|
>>>>>>       |
>>>>>>       ---eth3.228-fcoe
>>>>>>
>>>>>> This worked because of a change we added to allow inactive slaves
>>>>>> to rx 'exact' matches. This functionality was kept intact with the
>>>>>> rx_handler mechanism. However now the vlan interface attached to the
>>>>>> active slave never receives traffic because the bonding rx_handler
>>>>>> updates the skb->dev and goto's another_round. Previously, the
>>>>>> vlan_do_receive() logic was called before the bonding rx_handler.
>>>>>>
>>>>>> Now by the time vlan_do_receive calls vlan_find_dev() the
>>>>>> skb->dev is set to bond0 and it is clear no vlan is attached
>>>>>> to this iface. The vlan lookup fails.
>>>>>>
>>>>>> This patch moves the VLAN check above the rx_handler. A VLAN
>>>>>> tagged frame is now routed to the eth2.228-fcoe iface in the
>>>>>> above schematic. Untagged frames continue to the bond0 as
>>>>>> normal. This case also remains intact,
>>>>>>
>>>>>> eth2 --> bond0 --> vlan.228
>>>>>>
>>>>>> Here the skb is VLAN tagged but the vlan lookup fails on eth2
>>>>>> causing the bonding rx_handler to be called. On the second
>>>>>> pass the vlan lookup is on the bond0 iface and completes as
>>>>>> expected.
>>>>>>
>>>>>> Putting a VLAN.228 on both the bond0 and eth2 device will
>>>>>> result in eth2.228 receiving the skb. I don't think this is
>>>>>> completely unexpected and was the result prior to the rx_handler
>>>>>> result.
>>
>> I think this OK, but I do have a question
>> if bond0 is in Active/Backup mode, eth2 and eth3 got the same MAC.addr,
>> what about the VLAN:s ?
>> (or is just one of thme working ??)
>>
>
>The VLAN MAC address will not be managed by the bond. In the
>storage case a SAN mac may be used (NETDEV_HW_ADDR_T_SAN).
>Otherwise the MAC can be managed normally.
>
>Both VLANs will receive frames but in some modes only to packet
>handlers that have exact matches. See bond_should_deliver_exact_match().
>
>.John.

Have made some test now,  this patch solves a big issue that we had with VLANs 
i.e. as a work-a-round we put macvlans in between the phys. interface and the bond.
I have tested the scenario below, where tipc is running on VLAN below the bonding interface.
With the patch it works fine now.
If you want you can add a
Tested-by: Hans Schillstrom <hams.schillstrom@ericsson.com>

                      +---------+        +---------+
                    +---------+ |      +---------+ |
                  +---------+ |-+    +---------+ |-+
                  | macvlan |-+      | macvlan |-+
                  +---------+        +---------+
                     | | |              | | |
                     | | |           +---------+
                     | | |       ----|  vlan8  |
                     | | |      /    +---------+
                     | | |     /
                  +----+----+ /
        +---------|  bond0  |=------------+
        |         +---------+             |
        |                                 |
   +----+----+  +---------+          +----+----+  +---------+
   |   eth1  |--|  vlan20 |          |   eth2  |--|  vlan21 |
   +----+----+  +---------+          +----+----+  +---------+
        |                                 |
        |                                 |
  +-----+-----+                     +-----+-----+
  | Switch-0  |_____________________|   Sw1     |
  |           |    ISL TRUNK        |           |
  +-+---+---+-+                     +-+---+---+-+
    |   |   |                         |   |   |
  vlan1 | vlan20                    vlan1 | vlan21
      vlan8                             vlan8



Thanks 
Hans

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[John Fastabend]

On 10/10/2011 7:43 PM, Jesse Gross wrote:
> On Mon, Oct 10, 2011 at 7:07 PM, John Fastabend
> <john.r.fastabend@intel.com> wrote:
>> On 10/10/2011 3:37 PM, Jiri Pirko wrote:
>>> Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
>>>> The following configuration used to work as I expected. At least
>>>> we could use the fcoe interfaces to do MPIO and the bond0 iface
>>>> to do load balancing or failover.
>>>>
>>>>       ---eth2.228-fcoe
>>>>       |
>>>> eth2 -----|
>>>>          |
>>>>          |---- bond0
>>>>          |
>>>> eth3 -----|
>>>>       |
>>>>       ---eth3.228-fcoe
>>>>
>>>> This worked because of a change we added to allow inactive slaves
>>>> to rx 'exact' matches. This functionality was kept intact with the
>>>> rx_handler mechanism. However now the vlan interface attached to the
>>>> active slave never receives traffic because the bonding rx_handler
>>>> updates the skb->dev and goto's another_round. Previously, the
>>>> vlan_do_receive() logic was called before the bonding rx_handler.
>>>>
>>>> Now by the time vlan_do_receive calls vlan_find_dev() the
>>>> skb->dev is set to bond0 and it is clear no vlan is attached
>>>> to this iface. The vlan lookup fails.
>>>>
>>>> This patch moves the VLAN check above the rx_handler. A VLAN
>>>> tagged frame is now routed to the eth2.228-fcoe iface in the
>>>> above schematic. Untagged frames continue to the bond0 as
>>>> normal. This case also remains intact,
>>>>
>>>> eth2 --> bond0 --> vlan.228
>>>>
>>>> Here the skb is VLAN tagged but the vlan lookup fails on eth2
>>>> causing the bonding rx_handler to be called. On the second
>>>> pass the vlan lookup is on the bond0 iface and completes as
>>>> expected.
>>>>
>>>> Putting a VLAN.228 on both the bond0 and eth2 device will
>>>> result in eth2.228 receiving the skb. I don't think this is
>>>> completely unexpected and was the result prior to the rx_handler
>>>> result.
>>>>
>>>> Note, the same setup is also used for other storage traffic that
>>>> MPIO is used with eg. iSCSI and similar setups can be contrived
>>>> without storage protocols.
>>>>
>>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>>> ---

[...]

>> Maybe Jesse, can comment though on why this commit that moved (and
>> cleaned up) the vlan tag handling put the vlan_do_receive below the
>> rx_handler rather than above it. Was this intended to fix something?
> 
> The original reason was to ensure packets received from NICs that do
> stripping behaved the same as those that don't.  At the time, the
> packets with inline vlan tags were handled by the same code that
> handled upper layer protocols so it was important that code for vlan
> stripped tags be immediately before that.  Otherwise, packets might be
> taken either by the bridge hook or vlan code depending the the type of
> device.
> 
> However, that's no longer an issue because we now emulate vlan
> acceleration by untagging packets at the beginning of
> __netif_receive_skb(), so the code path will always be the same.
> Furthermore, based on feedback received since that patch it seems
> pretty clear that people prefer the behavior where vlan devices take
> traffic first, so now that we can have both that and consistent
> behavior it seems to be the way to go.
> 
> This looks correct to me:
> Acked-by: Jesse Gross <jesse@nicira.com>

Thanks for the nice summary Jesse.

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Jiri Pirko]

Tue, Oct 11, 2011 at 12:37:53AM CEST, jpirko@redhat.com wrote:
>Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:
>>The following configuration used to work as I expected. At least
>>we could use the fcoe interfaces to do MPIO and the bond0 iface
>>to do load balancing or failover.
>>
>>       ---eth2.228-fcoe
>>       |
>>eth2 -----|
>>          |
>>          |---- bond0
>>          |
>>eth3 -----|
>>       |
>>       ---eth3.228-fcoe
>>
>>This worked because of a change we added to allow inactive slaves
>>to rx 'exact' matches. This functionality was kept intact with the
>>rx_handler mechanism. However now the vlan interface attached to the
>>active slave never receives traffic because the bonding rx_handler
>>updates the skb->dev and goto's another_round. Previously, the
>>vlan_do_receive() logic was called before the bonding rx_handler.
>>
>>Now by the time vlan_do_receive calls vlan_find_dev() the
>>skb->dev is set to bond0 and it is clear no vlan is attached
>>to this iface. The vlan lookup fails.
>>
>>This patch moves the VLAN check above the rx_handler. A VLAN
>>tagged frame is now routed to the eth2.228-fcoe iface in the
>>above schematic. Untagged frames continue to the bond0 as
>>normal. This case also remains intact,
>>
>>eth2 --> bond0 --> vlan.228
>>
>>Here the skb is VLAN tagged but the vlan lookup fails on eth2
>>causing the bonding rx_handler to be called. On the second
>>pass the vlan lookup is on the bond0 iface and completes as
>>expected.
>>
>>Putting a VLAN.228 on both the bond0 and eth2 device will
>>result in eth2.228 receiving the skb. I don't think this is
>>completely unexpected and was the result prior to the rx_handler
>>result.
>>
>>Note, the same setup is also used for other storage traffic that
>>MPIO is used with eg. iSCSI and similar setups can be contrived
>>without storage protocols.
>>
>>Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>---
>>
>> net/core/dev.c |   22 +++++++++++-----------
>> 1 files changed, 11 insertions(+), 11 deletions(-)
>>
>>diff --git a/net/core/dev.c b/net/core/dev.c
>>index 70ecb86..8b6118a 100644
>>--- a/net/core/dev.c
>>+++ b/net/core/dev.c
>>@@ -3231,6 +3231,17 @@ another_round:
>> ncls:
>> #endif
>> 
>>+	if (vlan_tx_tag_present(skb)) {
>>+		if (pt_prev) {
>>+			ret = deliver_skb(skb, pt_prev, orig_dev);
>>+			pt_prev = NULL;
>>+		}
>>+		if (vlan_do_receive(&skb))
>>+			goto another_round;
>>+		else if (unlikely(!skb))
>>+			goto out;
>>+	}
>>+
>> 	rx_handler = rcu_dereference(skb->dev->rx_handler);
>> 	if (rx_handler) {
>> 		if (pt_prev) {
>>@@ -3251,17 +3262,6 @@ ncls:
>> 		}
>> 	}
>> 
>>-	if (vlan_tx_tag_present(skb)) {
>>-		if (pt_prev) {
>>-			ret = deliver_skb(skb, pt_prev, orig_dev);
>>-			pt_prev = NULL;
>>-		}
>>-		if (vlan_do_receive(&skb))
>>-			goto another_round;
>>-		else if (unlikely(!skb))
>>-			goto out;
>>-	}
>>-
>> 	/* deliver only exact match when indicated */
>> 	null_or_dev = deliver_exact ? skb->dev : NULL;
>> 
>>
>
>Hmm, I must look at this again tomorrow but I have strong feeling this
>will break some some scenario including vlan-bridge-macvlan.

I didn't find out anything.

Reviewed-by: Jiri Pirko <jpirko@redhat.com>

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Maxime Bizon]

On Tue, 2011-10-11 at 00:37 +0200, Jiri Pirko wrote:

> Hmm, I must look at this again tomorrow but I have strong feeling this
> will break some some scenario including vlan-bridge-macvlan. 

unless I'm mistaken, today's behaviour:

# vconfig add eth0 100
# brctl addbr br0
# brctl addif br0 eth0

=> eth0.100 gets no more packets, br0.100 is to be used

after the patch won't we get the opposite ?

-- 
Maxime

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Jiri Pirko]

Thu, Oct 13, 2011 at 05:04:34PM CEST, mbizon@freebox.fr wrote:
>
>On Tue, 2011-10-11 at 00:37 +0200, Jiri Pirko wrote:
>
>> Hmm, I must look at this again tomorrow but I have strong feeling this
>> will break some some scenario including vlan-bridge-macvlan. 
>
>unless I'm mistaken, today's behaviour:
>
># vconfig add eth0 100
># brctl addbr br0
># brctl addif br0 eth0
>
>=> eth0.100 gets no more packets, br0.100 is to be used
>
>after the patch won't we get the opposite ?

Looks like it. The question is what is the correct behaviour...

>
>-- 
>Maxime
>
>

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Maxime Bizon]

On Thu, 2011-10-13 at 17:38 +0200, Jiri Pirko wrote:

> Looks like it. The question is what is the correct behaviour... 

since we don't want to break existing setup I would say the current one

but I must admit I'm not a big fan of it.

-- 
Maxime

RE: [net-next PATCH] net: allow vlan traffic to be received under bond

[Hans Schillström]

Thu, Oct 13, 2011 at 05:04:34PM CEST, mbizon@freebox.fr wrote:
>>
>>On Tue, 2011-10-11 at 00:37 +0200, Jiri Pirko wrote:
>>
>>> Hmm, I must look at this again tomorrow but I have strong feeling this
>>> will break some some scenario including vlan-bridge-macvlan.
>>
>>unless I'm mistaken, today's behaviour:
>>
>># vconfig add eth0 100
>># brctl addbr br0
>># brctl addif br0 eth0
>>
>>=> eth0.100 gets no more packets, br0.100 is to be used
>>
>>after the patch won't we get the opposite ?
>
>Looks like it. The question is what is the correct behaviour...

I think this it become correct now, you should not destroy lover level if possible.
I.e. as John wrote "it's not an unexpected behaviour"

Consider adding a bridge to a vlan like this

vconfig add eth0 100
brctl addbr br1
brctl addif br1 eth0.100

If you later add a bridge (or bond) should the previous added bridge still work ? 
Yes I think so, for me it's the expected behaviour.

brctl addbr br0
brctl addif br0 eth0

Regards
Hans

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[John Fastabend]

On 10/13/2011 8:59 AM, Hans Schillström wrote:
> Thu, Oct 13, 2011 at 05:04:34PM CEST, mbizon@freebox.fr wrote:
>>>
>>> On Tue, 2011-10-11 at 00:37 +0200, Jiri Pirko wrote:
>>>
>>>> Hmm, I must look at this again tomorrow but I have strong feeling this
>>>> will break some some scenario including vlan-bridge-macvlan.
>>>
>>> unless I'm mistaken, today's behaviour:
>>>
>>> # vconfig add eth0 100
>>> # brctl addbr br0
>>> # brctl addif br0 eth0
>>>
>>> => eth0.100 gets no more packets, br0.100 is to be used
>>>
>>> after the patch won't we get the opposite ?
>>
>> Looks like it. The question is what is the correct behaviour...
> 
> I think this it become correct now, you should not destroy lover level if possible.
> I.e. as John wrote "it's not an unexpected behaviour"
> 
> Consider adding a bridge to a vlan like this
> 
> vconfig add eth0 100
> brctl addbr br1
> brctl addif br1 eth0.100
> 
> If you later add a bridge (or bond) should the previous added bridge still work ? 
> Yes I think so, for me it's the expected behaviour.
> 
> brctl addbr br0
> brctl addif br0 eth0
> 
> Regards
> Hans
> 
> 

Sorry I'm not entirely sure I followed the above two posts. Note
this patch restores behavior that has existed for most of the
2.6.x kernels. In the 3.x kernels VLAN100 is dropped in the
schematic below (assuming eth0 is active), I think this is
incorrect.

       ---> eth0.100
       |
       |
eth0 -----> br0

With this patch VLAN100 is delivered to eth0.100 as I expect. Now
adding a VLAN100 to br0.


       ---> eth0.100
       |
       |
eth0 -----> br0---> br0.100

With this patch in the above case VLAN100 is delivered to the
first matching vlan. Here eth0.100 will receive the packet If
you really want br0.100 to receive the packet remove eth0.100.
Without this patch the packet will be delivered to br0.100,
but no configuration will allow a packet to be delivered to
eth0.100 with a bond.

The first schematic is used for doing bonding on LAN traffic
and SAN (storage) traffic with MPIO. So I think my expectations
are correct and have a real use case.

Thanks,
John

RE: [net-next PATCH] net: allow vlan traffic to be received under bond

[Hans Schillström]

>On 10/13/2011 8:59 AM, Hans Schillström wrote:
>> Thu, Oct 13, 2011 at 05:04:34PM CEST, mbizon@freebox.fr wrote:
>>>>
>>>> On Tue, 2011-10-11 at 00:37 +0200, Jiri Pirko wrote:
>>>>
>>>>> Hmm, I must look at this again tomorrow but I have strong feeling this
>>>>> will break some some scenario including vlan-bridge-macvlan.
>>>>
>>>> unless I'm mistaken, today's behaviour:
>>>>
>>>> # vconfig add eth0 100
>>>> # brctl addbr br0
>>>> # brctl addif br0 eth0
>>>>
>>>> => eth0.100 gets no more packets, br0.100 is to be used
>>>>
>>>> after the patch won't we get the opposite ?
>>>
>>> Looks like it. The question is what is the correct behaviour...
>>
>> I think this it become correct now, you should not destroy lover level if possible.
>> I.e. as John wrote "it's not an unexpected behaviour"
>>
>> Consider adding a bridge to a vlan like this
>>
>> vconfig add eth0 100
>> brctl addbr br1
>> brctl addif br1 eth0.100
>>
>> If you later add a bridge (or bond) should the previous added bridge still work ?
>> Yes I think so, for me it's the expected behaviour.
>>
>> brctl addbr br0
>> brctl addif br0 eth0
>>
>
>Sorry I'm not entirely sure I followed the above two posts. Note
>this patch restores behavior that has existed for most of the
>2.6.x kernels. In the 3.x kernels VLAN100 is dropped in the
>schematic below (assuming eth0 is active), I think this is
>incorrect.
>
>      ---> eth0.100
>       |
>       |
>eth0 -----> br0
>
>With this patch VLAN100 is delivered to eth0.100 as I expect. Now
>adding a VLAN100 to br0.
>
>
>       ---> eth0.100
>       |
>       |
>eth0 -----> br0---> br0.100
>
>With this patch in the above case VLAN100 is delivered to the
>first matching vlan. Here eth0.100 will receive the packet If
>you really want br0.100 to receive the packet remove eth0.100.
>Without this patch the packet will be delivered to br0.100,
>but no configuration will allow a packet to be delivered to
>eth0.100 with a bond.
>
>The first schematic is used for doing bonding on LAN traffic
>and SAN (storage) traffic with MPIO. So I think my expectations
>are correct and have a real use case.
>
>Thanks,
>John

Sorry if I caused confusion,  I do agree with you to 100%.

Regards
Hans

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Jesse Gross]

2011/10/13 John Fastabend <john.r.fastabend@intel.com>:
> On 10/13/2011 8:59 AM, Hans Schillström wrote:
>> Thu, Oct 13, 2011 at 05:04:34PM CEST, mbizon@freebox.fr wrote:
>>>>
>>>> On Tue, 2011-10-11 at 00:37 +0200, Jiri Pirko wrote:
>>>>
>>>>> Hmm, I must look at this again tomorrow but I have strong feeling this
>>>>> will break some some scenario including vlan-bridge-macvlan.
>>>>
>>>> unless I'm mistaken, today's behaviour:
>>>>
>>>> # vconfig add eth0 100
>>>> # brctl addbr br0
>>>> # brctl addif br0 eth0
>>>>
>>>> => eth0.100 gets no more packets, br0.100 is to be used
>>>>
>>>> after the patch won't we get the opposite ?
>>>
>>> Looks like it. The question is what is the correct behaviour...
>>
>> I think this it become correct now, you should not destroy lover level if possible.
>> I.e. as John wrote "it's not an unexpected behaviour"
>>
>> Consider adding a bridge to a vlan like this
>>
>> vconfig add eth0 100
>> brctl addbr br1
>> brctl addif br1 eth0.100
>>
>> If you later add a bridge (or bond) should the previous added bridge still work ?
>> Yes I think so, for me it's the expected behaviour.
>>
>> brctl addbr br0
>> brctl addif br0 eth0
>>
>> Regards
>> Hans
>>
>>
>
> Sorry I'm not entirely sure I followed the above two posts. Note
> this patch restores behavior that has existed for most of the
> 2.6.x kernels. In the 3.x kernels VLAN100 is dropped in the
> schematic below (assuming eth0 is active), I think this is
> incorrect.

Actually, for most of 2.6.x the behavior was somewhat
non-deterministic since it depended on kernel version and the NIC.  As
a result, I think we can safely say that this wasn't a particularly
firm interface that we have to be wedded to.  Based on overwhelming
feedback, I think the interface in this patch is the preferred one and
what we should stabilize on.

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Thu, 13 Oct 2011 17:22:02 -0700

> Actually, for most of 2.6.x the behavior was somewhat
> non-deterministic since it depended on kernel version and the NIC.  As
> a result, I think we can safely say that this wasn't a particularly
> firm interface that we have to be wedded to.  Based on overwhelming
> feedback, I think the interface in this patch is the preferred one and
> what we should stabilize on.

Agreed, and I've applied this patch to net-next, thanks everyone!

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Eric Dumazet]

Le mardi 18 octobre 2011 à 23:47 -0400, David Miller a écrit :
> From: Jesse Gross <jesse@nicira.com>
> Date: Thu, 13 Oct 2011 17:22:02 -0700
> 
> > Actually, for most of 2.6.x the behavior was somewhat
> > non-deterministic since it depended on kernel version and the NIC.  As
> > a result, I think we can safely say that this wasn't a particularly
> > firm interface that we have to be wedded to.  Based on overwhelming
> > feedback, I think the interface in this patch is the preferred one and
> > what we should stabilize on.
> 
> Agreed, and I've applied this patch to net-next, thanks everyone!

Hmm.

Oh well, this broke my setup, a very basic one.

eth1 and eth2 on a bonding device, bond0, active-backup

some vlans on top of bond0, say vlan.103

$ ip link show dev vlan.103
8: vlan.103@bond0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
pfifo_fast state UP qlen 100
    link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff


arp_rcv() now gets packets with skb->type PACKET_OTHERHOST and drops
such packets.

     [000] 52870.115435: skb_gro_reset_offset <-napi_gro_receive
     [000] 52870.115435: dev_gro_receive <-napi_gro_receive
     [000] 52870.115435: napi_skb_finish <-napi_gro_receive
     [000] 52870.115435: netif_receive_skb <-napi_skb_finish
     [000] 52870.115435: get_rps_cpu <-netif_receive_skb
     [000] 52870.115435: __netif_receive_skb <-netif_receive_skb
     [000] 52870.115436: vlan_do_receive <-__netif_receive_skb
     [000] 52870.115436: bond_handle_frame <-__netif_receive_skb
     [000] 52870.115436: vlan_do_receive <-__netif_receive_skb
     [000] 52870.115436: arp_rcv <-__netif_receive_skb
     [000] 52870.115436: kfree_skb <-arp_rcv
     [000] 52870.115437: __kfree_skb <-kfree_skb
     [000] 52870.115437: skb_release_head_state <-__kfree_skb
     [000] 52870.115437: skb_release_data <-__kfree_skb
     [000] 52870.115437: kfree <-skb_release_data
     [000] 52870.115437: kmem_cache_free <-__kfree_skb


By the way, we have no SNMP counter here so I spent some time to track
this. I'll send a patch for this.

If this host initiates the trafic, all is well.

Please guys, can we get back ARP or revert this patch ?

Thanks

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Eric Dumazet]

Le vendredi 28 octobre 2011 à 12:00 +0200, Eric Dumazet a écrit :

> Oh well, this broke my setup, a very basic one.
> 
> eth1 and eth2 on a bonding device, bond0, active-backup
> 
> some vlans on top of bond0, say vlan.103
> 
> $ ip link show dev vlan.103
> 8: vlan.103@bond0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
> pfifo_fast state UP qlen 100
>     link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff
> 
> 
> arp_rcv() now gets packets with skb->type PACKET_OTHERHOST and drops
> such packets.
> 
>      [000] 52870.115435: skb_gro_reset_offset <-napi_gro_receive
>      [000] 52870.115435: dev_gro_receive <-napi_gro_receive
>      [000] 52870.115435: napi_skb_finish <-napi_gro_receive
>      [000] 52870.115435: netif_receive_skb <-napi_skb_finish
>      [000] 52870.115435: get_rps_cpu <-netif_receive_skb
>      [000] 52870.115435: __netif_receive_skb <-netif_receive_skb
>      [000] 52870.115436: vlan_do_receive <-__netif_receive_skb
>      [000] 52870.115436: bond_handle_frame <-__netif_receive_skb
>      [000] 52870.115436: vlan_do_receive <-__netif_receive_skb
>      [000] 52870.115436: arp_rcv <-__netif_receive_skb
>      [000] 52870.115436: kfree_skb <-arp_rcv
>      [000] 52870.115437: __kfree_skb <-kfree_skb
>      [000] 52870.115437: skb_release_head_state <-__kfree_skb
>      [000] 52870.115437: skb_release_data <-__kfree_skb
>      [000] 52870.115437: kfree <-skb_release_data
>      [000] 52870.115437: kmem_cache_free <-__kfree_skb
> 
> 
> By the way, we have no SNMP counter here so I spent some time to track
> this. I'll send a patch for this.
> 
> If this host initiates the trafic, all is well.
> 
> Please guys, can we get back ARP or revert this patch ?

Following patch cures the problem, I am not sure its the right fix.

Problem is we dont know how many times vlan_do_receive() can be called
for a packet.

Only last call should set/mess pkt_type to PACKET_OTHERHOST.

So the caller should be responsible for this, not vlan_do_receive()


Alternative would be to check skb->dev->rx_handler being NULL,
but its not clean.

Following patch is a hack because it handles multicast/broadcast trafic
only. Unicast is already handled in lines 26-33, this is why we didnt
catch the problem.

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f1f2f7b..6861899 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -13,7 +13,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
 
 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
 	if (!vlan_dev) {
-		if (vlan_id)
+		if (vlan_id && skb->pkt_type == PACKET_HOST)
 			skb->pkt_type = PACKET_OTHERHOST;
 		return false;
 	}

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[John Fastabend]

On 10/28/2011 4:06 AM, Eric Dumazet wrote:
> Le vendredi 28 octobre 2011 à 12:00 +0200, Eric Dumazet a écrit :
> 
>> Oh well, this broke my setup, a very basic one.
>>
>> eth1 and eth2 on a bonding device, bond0, active-backup
>>
>> some vlans on top of bond0, say vlan.103
>>
>> $ ip link show dev vlan.103
>> 8: vlan.103@bond0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
>> pfifo_fast state UP qlen 100
>>     link/ether 00:1e:0b:ec:d3:d2 brd ff:ff:ff:ff:ff:ff
>>
>>
>> arp_rcv() now gets packets with skb->type PACKET_OTHERHOST and drops
>> such packets.
>>
>>      [000] 52870.115435: skb_gro_reset_offset <-napi_gro_receive
>>      [000] 52870.115435: dev_gro_receive <-napi_gro_receive
>>      [000] 52870.115435: napi_skb_finish <-napi_gro_receive
>>      [000] 52870.115435: netif_receive_skb <-napi_skb_finish
>>      [000] 52870.115435: get_rps_cpu <-netif_receive_skb
>>      [000] 52870.115435: __netif_receive_skb <-netif_receive_skb
>>      [000] 52870.115436: vlan_do_receive <-__netif_receive_skb
>>      [000] 52870.115436: bond_handle_frame <-__netif_receive_skb
>>      [000] 52870.115436: vlan_do_receive <-__netif_receive_skb
>>      [000] 52870.115436: arp_rcv <-__netif_receive_skb
>>      [000] 52870.115436: kfree_skb <-arp_rcv
>>      [000] 52870.115437: __kfree_skb <-kfree_skb
>>      [000] 52870.115437: skb_release_head_state <-__kfree_skb
>>      [000] 52870.115437: skb_release_data <-__kfree_skb
>>      [000] 52870.115437: kfree <-skb_release_data
>>      [000] 52870.115437: kmem_cache_free <-__kfree_skb
>>
>>
>> By the way, we have no SNMP counter here so I spent some time to track
>> this. I'll send a patch for this.
>>
>> If this host initiates the trafic, all is well.
>>
>> Please guys, can we get back ARP or revert this patch ?
> 
> Following patch cures the problem, I am not sure its the right fix.
> 
> Problem is we dont know how many times vlan_do_receive() can be called
> for a packet.
> 
> Only last call should set/mess pkt_type to PACKET_OTHERHOST.
> 
> So the caller should be responsible for this, not vlan_do_receive()
> 
> 
> Alternative would be to check skb->dev->rx_handler being NULL,
> but its not clean.
> 
> Following patch is a hack because it handles multicast/broadcast trafic
> only. Unicast is already handled in lines 26-33, this is why we didnt
> catch the problem.
> 
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index f1f2f7b..6861899 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -13,7 +13,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
>  
>  	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
>  	if (!vlan_dev) {
> -		if (vlan_id)
> +		if (vlan_id && skb->pkt_type == PACKET_HOST)
>  			skb->pkt_type = PACKET_OTHERHOST;
>  		return false;
>  	}
> 

Thanks Eric! Thought about this some and I haven't come up
with anything better yet. Even though this might be a slight
hack I would prefer this to reverting the patch.

I'll think about this more tomorrow. Would you be against
submitting this patch?

.John

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Eric Dumazet]

Le vendredi 28 octobre 2011 à 19:20 -0700, John Fastabend a écrit :

> Thanks Eric! Thought about this some and I haven't come up
> with anything better yet. Even though this might be a slight
> hack I would prefer this to reverting the patch.
> 
> I'll think about this more tomorrow. Would you be against
> submitting this patch?

I cant submit this patch, because its a hack and partial fix.

For Unicast packets, we still do the wrong thing : setting their
pkt_type to PACKET_OTHERHOST before the call to rx_handler :

In this case, bond_handle_frame() wont handle this packet correctly in
some cases (BOND_MODE_ALB ...). I suppose bridge might be confused as
well. So other problems remain.

We should delay the PACKET_OTHERHOST setting to the last moment, that is
the last time vlan_do_receive() is called.

What about following patch instead ?

[PATCH] vlan: allow nested vlan_do_receive()

commit 2425717b27eb (net: allow vlan traffic to be received under bond)
broke ARP processing on vlan on top of bonding.

       +-------+
eth0 --| bond0 |---bond0.103
eth1 --|       |
       +-------+

52870.115435: skb_gro_reset_offset <-napi_gro_receive
52870.115435: dev_gro_receive <-napi_gro_receive
52870.115435: napi_skb_finish <-napi_gro_receive
52870.115435: netif_receive_skb <-napi_skb_finish
52870.115435: get_rps_cpu <-netif_receive_skb
52870.115435: __netif_receive_skb <-netif_receive_skb
52870.115436: vlan_do_receive <-__netif_receive_skb
52870.115436: bond_handle_frame <-__netif_receive_skb
52870.115436: vlan_do_receive <-__netif_receive_skb
52870.115436: arp_rcv <-__netif_receive_skb
52870.115436: kfree_skb <-arp_rcv

Packet is dropped in arp_rcv() because its pkt_type was set to
PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
exists.

We really need to change pkt_type only if no more rx_handler is about to
be called for the packet.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/if_vlan.h |    8 +++++---
 net/8021q/vlan_core.c   |    7 +++++--
 net/core/dev.c          |    4 ++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 44da482..95874ff 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -106,7 +106,8 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
 extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
 extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
-extern bool vlan_do_receive(struct sk_buff **skb);
+extern bool vlan_do_receive(struct sk_buff **skb,
+			    rx_handler_func_t *rx_handler);
 extern struct sk_buff *vlan_untag(struct sk_buff *skb);
 
 #else
@@ -128,9 +129,10 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
 	return 0;
 }
 
-static inline bool vlan_do_receive(struct sk_buff **skb)
+static inline bool vlan_do_receive(struct sk_buff **skb,
+				   rx_handler_func_t *rx_handler)
 {
-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && !rx_handler)
 		(*skb)->pkt_type = PACKET_OTHERHOST;
 	return false;
 }
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f1f2f7b..3ec1ada 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -4,7 +4,7 @@
 #include <linux/netpoll.h>
 #include "vlan.h"
 
-bool vlan_do_receive(struct sk_buff **skbp)
+bool vlan_do_receive(struct sk_buff **skbp, rx_handler_func_t *rx_handler)
 {
 	struct sk_buff *skb = *skbp;
 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
 
 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
 	if (!vlan_dev) {
-		if (vlan_id)
+		/* Only the last call to vlan_do_receive() should change
+		 * pkt_type to PACKET_OTHERHOST
+		 */
+		if (vlan_id && !rx_handler)
 			skb->pkt_type = PACKET_OTHERHOST;
 		return false;
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index edcf019..40976b4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3283,18 +3283,18 @@ another_round:
 ncls:
 #endif
 
+	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (vlan_tx_tag_present(skb)) {
 		if (pt_prev) {
 			ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = NULL;
 		}
-		if (vlan_do_receive(&skb))
+		if (vlan_do_receive(&skb, rx_handler))
 			goto another_round;
 		else if (unlikely(!skb))
 			goto out;
 	}
 
-	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (rx_handler) {
 		if (pt_prev) {
 			ret = deliver_skb(skb, pt_prev, orig_dev);

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Jiri Pirko]

Sat, Oct 29, 2011 at 12:22:26PM CEST, eric.dumazet@gmail.com wrote:
>Le vendredi 28 octobre 2011 à 19:20 -0700, John Fastabend a écrit :
>
>> Thanks Eric! Thought about this some and I haven't come up
>> with anything better yet. Even though this might be a slight
>> hack I would prefer this to reverting the patch.
>> 
>> I'll think about this more tomorrow. Would you be against
>> submitting this patch?
>
>I cant submit this patch, because its a hack and partial fix.
>
>For Unicast packets, we still do the wrong thing : setting their
>pkt_type to PACKET_OTHERHOST before the call to rx_handler :
>
>In this case, bond_handle_frame() wont handle this packet correctly in
>some cases (BOND_MODE_ALB ...). I suppose bridge might be confused as
>well. So other problems remain.
>
>We should delay the PACKET_OTHERHOST setting to the last moment, that is
>the last time vlan_do_receive() is called.
>
>What about following patch instead ?
>
>[PATCH] vlan: allow nested vlan_do_receive()
>
>commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>broke ARP processing on vlan on top of bonding.
>
>       +-------+
>eth0 --| bond0 |---bond0.103
>eth1 --|       |
>       +-------+
>
>52870.115435: skb_gro_reset_offset <-napi_gro_receive
>52870.115435: dev_gro_receive <-napi_gro_receive
>52870.115435: napi_skb_finish <-napi_gro_receive
>52870.115435: netif_receive_skb <-napi_skb_finish
>52870.115435: get_rps_cpu <-netif_receive_skb
>52870.115435: __netif_receive_skb <-netif_receive_skb
>52870.115436: vlan_do_receive <-__netif_receive_skb
>52870.115436: bond_handle_frame <-__netif_receive_skb
>52870.115436: vlan_do_receive <-__netif_receive_skb
>52870.115436: arp_rcv <-__netif_receive_skb
>52870.115436: kfree_skb <-arp_rcv
>
>Packet is dropped in arp_rcv() because its pkt_type was set to
>PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>exists.
>
>We really need to change pkt_type only if no more rx_handler is about to
>be called for the packet.
>
>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>---
> include/linux/if_vlan.h |    8 +++++---
> net/8021q/vlan_core.c   |    7 +++++--
> net/core/dev.c          |    4 ++--
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
>diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>index 44da482..95874ff 100644
>--- a/include/linux/if_vlan.h
>+++ b/include/linux/if_vlan.h
>@@ -106,7 +106,8 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
> extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
> extern u16 vlan_dev_vlan_id(const struct net_device *dev);
> 
>-extern bool vlan_do_receive(struct sk_buff **skb);
>+extern bool vlan_do_receive(struct sk_buff **skb,
>+			    rx_handler_func_t *rx_handler);
> extern struct sk_buff *vlan_untag(struct sk_buff *skb);
> 
> #else
>@@ -128,9 +129,10 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
> 	return 0;
> }
> 
>-static inline bool vlan_do_receive(struct sk_buff **skb)
>+static inline bool vlan_do_receive(struct sk_buff **skb,
>+				   rx_handler_func_t *rx_handler)
> {
>-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && !rx_handler)
> 		(*skb)->pkt_type = PACKET_OTHERHOST;
> 	return false;
> }
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index f1f2f7b..3ec1ada 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -4,7 +4,7 @@
> #include <linux/netpoll.h>
> #include "vlan.h"
> 
>-bool vlan_do_receive(struct sk_buff **skbp)
>+bool vlan_do_receive(struct sk_buff **skbp, rx_handler_func_t *rx_handler)
> {
> 	struct sk_buff *skb = *skbp;
> 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
> 
> 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
> 	if (!vlan_dev) {
>-		if (vlan_id)
>+		/* Only the last call to vlan_do_receive() should change
>+		 * pkt_type to PACKET_OTHERHOST
>+		 */
>+		if (vlan_id && !rx_handler)
> 			skb->pkt_type = PACKET_OTHERHOST;
> 		return false;
> 	}
>diff --git a/net/core/dev.c b/net/core/dev.c
>index edcf019..40976b4 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -3283,18 +3283,18 @@ another_round:
> ncls:
> #endif
> 
>+	rx_handler = rcu_dereference(skb->dev->rx_handler);
> 	if (vlan_tx_tag_present(skb)) {
> 		if (pt_prev) {
> 			ret = deliver_skb(skb, pt_prev, orig_dev);
> 			pt_prev = NULL;
> 		}
>-		if (vlan_do_receive(&skb))
>+		if (vlan_do_receive(&skb, rx_handler))

I must say I do not like passing rx_handler out like this. Apart it's
not nice, it might be misleading....

How about something like following instead? I must test it but I believe
it should resolve the problem.


diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 44da482..165a487 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -130,8 +130,6 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
 
 static inline bool vlan_do_receive(struct sk_buff **skb)
 {
-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
-		(*skb)->pkt_type = PACKET_OTHERHOST;
 	return false;
 }
 
@@ -141,6 +139,14 @@ static inline struct sk_buff *vlan_untag(struct sk_buff *skb)
 }
 #endif
 
+static inline void vlan_handle_leftover(struct sk_buff *skb)
+{
+	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
+
+	if (vlan_id)
+		skb->pkt_type = PACKET_OTHERHOST;
+}
+
 /**
  * vlan_insert_tag - regular VLAN tag inserting
  * @skb: skbuff to tag
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f1f2f7b..540da12 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -12,11 +12,8 @@ bool vlan_do_receive(struct sk_buff **skbp)
 	struct vlan_pcpu_stats *rx_stats;
 
 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
-	if (!vlan_dev) {
-		if (vlan_id)
-			skb->pkt_type = PACKET_OTHERHOST;
+	if (!vlan_dev)
 		return false;
-	}
 
 	skb = *skbp = skb_share_check(skb, GFP_ATOMIC);
 	if (unlikely(!skb))
diff --git a/net/core/dev.c b/net/core/dev.c
index b7ba81a..6fdfcc9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3314,6 +3314,14 @@ ncls:
 		}
 	}
 
+	if (vlan_tx_tag_present(skb)) {
+		/*
+		 * Tag is still present here. That means there's no device
+		 * set up for this vlan id. So handle these leftovers here.
+		 */
+		vlan_handle_leftover(skb);
+	}
+
 	/* deliver only exact match when indicated */
 	null_or_dev = deliver_exact ? skb->dev : NULL;
 

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Eric Dumazet]

Le samedi 29 octobre 2011 à 16:59 +0200, Jiri Pirko a écrit :
> Sat, Oct 29, 2011 at 12:22:26PM CEST, eric.dumazet@gmail.com wrote:
> >Le vendredi 28 octobre 2011 à 19:20 -0700, John Fastabend a écrit :
> >
> >> Thanks Eric! Thought about this some and I haven't come up
> >> with anything better yet. Even though this might be a slight
> >> hack I would prefer this to reverting the patch.
> >> 
> >> I'll think about this more tomorrow. Would you be against
> >> submitting this patch?
> >
> >I cant submit this patch, because its a hack and partial fix.
> >
> >For Unicast packets, we still do the wrong thing : setting their
> >pkt_type to PACKET_OTHERHOST before the call to rx_handler :
> >
> >In this case, bond_handle_frame() wont handle this packet correctly in
> >some cases (BOND_MODE_ALB ...). I suppose bridge might be confused as
> >well. So other problems remain.
> >
> >We should delay the PACKET_OTHERHOST setting to the last moment, that is
> >the last time vlan_do_receive() is called.
> >
> >What about following patch instead ?
> >
> >[PATCH] vlan: allow nested vlan_do_receive()
> >
> >commit 2425717b27eb (net: allow vlan traffic to be received under bond)
> >broke ARP processing on vlan on top of bonding.
> >
> >       +-------+
> >eth0 --| bond0 |---bond0.103
> >eth1 --|       |
> >       +-------+
> >
> >52870.115435: skb_gro_reset_offset <-napi_gro_receive
> >52870.115435: dev_gro_receive <-napi_gro_receive
> >52870.115435: napi_skb_finish <-napi_gro_receive
> >52870.115435: netif_receive_skb <-napi_skb_finish
> >52870.115435: get_rps_cpu <-netif_receive_skb
> >52870.115435: __netif_receive_skb <-netif_receive_skb
> >52870.115436: vlan_do_receive <-__netif_receive_skb
> >52870.115436: bond_handle_frame <-__netif_receive_skb
> >52870.115436: vlan_do_receive <-__netif_receive_skb
> >52870.115436: arp_rcv <-__netif_receive_skb
> >52870.115436: kfree_skb <-arp_rcv
> >
> >Packet is dropped in arp_rcv() because its pkt_type was set to
> >PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
> >exists.
> >
> >We really need to change pkt_type only if no more rx_handler is about to
> >be called for the packet.
> >
> >Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >---
> > include/linux/if_vlan.h |    8 +++++---
> > net/8021q/vlan_core.c   |    7 +++++--
> > net/core/dev.c          |    4 ++--
> > 3 files changed, 12 insertions(+), 7 deletions(-)
> >
> >diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> >index 44da482..95874ff 100644
> >--- a/include/linux/if_vlan.h
> >+++ b/include/linux/if_vlan.h
> >@@ -106,7 +106,8 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
> > extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
> > extern u16 vlan_dev_vlan_id(const struct net_device *dev);
> > 
> >-extern bool vlan_do_receive(struct sk_buff **skb);
> >+extern bool vlan_do_receive(struct sk_buff **skb,
> >+			    rx_handler_func_t *rx_handler);
> > extern struct sk_buff *vlan_untag(struct sk_buff *skb);
> > 
> > #else
> >@@ -128,9 +129,10 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
> > 	return 0;
> > }
> > 
> >-static inline bool vlan_do_receive(struct sk_buff **skb)
> >+static inline bool vlan_do_receive(struct sk_buff **skb,
> >+				   rx_handler_func_t *rx_handler)
> > {
> >-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
> >+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && !rx_handler)
> > 		(*skb)->pkt_type = PACKET_OTHERHOST;
> > 	return false;
> > }
> >diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> >index f1f2f7b..3ec1ada 100644
> >--- a/net/8021q/vlan_core.c
> >+++ b/net/8021q/vlan_core.c
> >@@ -4,7 +4,7 @@
> > #include <linux/netpoll.h>
> > #include "vlan.h"
> > 
> >-bool vlan_do_receive(struct sk_buff **skbp)
> >+bool vlan_do_receive(struct sk_buff **skbp, rx_handler_func_t *rx_handler)
> > {
> > 	struct sk_buff *skb = *skbp;
> > 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
> >@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
> > 
> > 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
> > 	if (!vlan_dev) {
> >-		if (vlan_id)
> >+		/* Only the last call to vlan_do_receive() should change
> >+		 * pkt_type to PACKET_OTHERHOST
> >+		 */
> >+		if (vlan_id && !rx_handler)
> > 			skb->pkt_type = PACKET_OTHERHOST;
> > 		return false;
> > 	}
> >diff --git a/net/core/dev.c b/net/core/dev.c
> >index edcf019..40976b4 100644
> >--- a/net/core/dev.c
> >+++ b/net/core/dev.c
> >@@ -3283,18 +3283,18 @@ another_round:
> > ncls:
> > #endif
> > 
> >+	rx_handler = rcu_dereference(skb->dev->rx_handler);
> > 	if (vlan_tx_tag_present(skb)) {
> > 		if (pt_prev) {
> > 			ret = deliver_skb(skb, pt_prev, orig_dev);
> > 			pt_prev = NULL;
> > 		}
> >-		if (vlan_do_receive(&skb))
> >+		if (vlan_do_receive(&skb, rx_handler))
> 
> I must say I do not like passing rx_handler out like this. Apart it's
> not nice, it might be misleading....
> 
> How about something like following instead? I must test it but I believe
> it should resolve the problem.
> 
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 44da482..165a487 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -130,8 +130,6 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
>  
>  static inline bool vlan_do_receive(struct sk_buff **skb)
>  {
> -	if ((*skb)->vlan_tci & VLAN_VID_MASK)
> -		(*skb)->pkt_type = PACKET_OTHERHOST;
>  	return false;
>  }
>  
> @@ -141,6 +139,14 @@ static inline struct sk_buff *vlan_untag(struct sk_buff *skb)
>  }
>  #endif
>  
> +static inline void vlan_handle_leftover(struct sk_buff *skb)
> +{
> +	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
> +
> +	if (vlan_id)
> +		skb->pkt_type = PACKET_OTHERHOST;
> +}
> +
>  /**
>   * vlan_insert_tag - regular VLAN tag inserting
>   * @skb: skbuff to tag
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index f1f2f7b..540da12 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -12,11 +12,8 @@ bool vlan_do_receive(struct sk_buff **skbp)
>  	struct vlan_pcpu_stats *rx_stats;
>  
>  	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
> -	if (!vlan_dev) {
> -		if (vlan_id)
> -			skb->pkt_type = PACKET_OTHERHOST;
> +	if (!vlan_dev)
>  		return false;
> -	}
>  
>  	skb = *skbp = skb_share_check(skb, GFP_ATOMIC);
>  	if (unlikely(!skb))
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b7ba81a..6fdfcc9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3314,6 +3314,14 @@ ncls:
>  		}
>  	}
>  
> +	if (vlan_tx_tag_present(skb)) {
> +		/*
> +		 * Tag is still present here. That means there's no device
> +		 * set up for this vlan id. So handle these leftovers here.
> +		 */
> +		vlan_handle_leftover(skb);
> +	}
> +
>  	/* deliver only exact match when indicated */
>  	null_or_dev = deliver_exact ? skb->dev : NULL;
>  

Hmm, is it really working ? where vlan_tci is cleared ?

This is indeed nice but adds another test in fast path, while in my
patch, additional tests are done in slow path only.

I see nothing wrong with passing rx_handler : Its probably cleaner than
adding rcu_dereference_raw(skb->dev->rx_handler) in the two
vlan_do_receive() implementations...

For reference, this was the first patch I had in mind, before I decided
that caller had to pass the rx_handler instead. (It could be a boolean
instead)

 include/linux/if_vlan.h |    3 ++-
 net/8021q/vlan_core.c   |    5 ++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 44da482..c6c00b4 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -130,7 +130,8 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
 
 static inline bool vlan_do_receive(struct sk_buff **skb)
 {
-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
+	if (((*skb)->vlan_tci & VLAN_VID_MASK) &&
+	    !rcu_dereference_raw((*skb)->dev->rx_handler))
 		(*skb)->pkt_type = PACKET_OTHERHOST;
 	return false;
 }
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f1f2f7b..245efb8 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
 
 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
 	if (!vlan_dev) {
-		if (vlan_id)
+		/* Only the last call to vlan_do_receive() should change
+		 * pkt_type to PACKET_OTHERHOST
+		 */
+		if (vlan_id && !rcu_dereference_raw(skb->dev->rx_handler))
 			skb->pkt_type = PACKET_OTHERHOST;
 		return false;
 	}

[PATCH v2] vlan: allow nested vlan_do_receive()

[Eric Dumazet]

commit 2425717b27eb (net: allow vlan traffic to be received under bond)
broke ARP processing on vlan on top of bonding.

       +-------+
eth0 --| bond0 |---bond0.103
eth1 --|       |
       +-------+

52870.115435: skb_gro_reset_offset <-napi_gro_receive
52870.115435: dev_gro_receive <-napi_gro_receive
52870.115435: napi_skb_finish <-napi_gro_receive
52870.115435: netif_receive_skb <-napi_skb_finish
52870.115435: get_rps_cpu <-netif_receive_skb
52870.115435: __netif_receive_skb <-netif_receive_skb
52870.115436: vlan_do_receive <-__netif_receive_skb
52870.115436: bond_handle_frame <-__netif_receive_skb
52870.115436: vlan_do_receive <-__netif_receive_skb
52870.115436: arp_rcv <-__netif_receive_skb
52870.115436: kfree_skb <-arp_rcv

Packet is dropped in arp_rcv() because its pkt_type was set to
PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
exists.

We really need to change pkt_type only if no more rx_handler is about to
be called for the packet.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
V2 : change the vlan_do_receive() added argument to be a boolean

 include/linux/if_vlan.h |    6 +++---
 net/8021q/vlan_core.c   |    7 +++++--
 net/core/dev.c          |    4 ++--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 44da482..12d5543 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -106,7 +106,7 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
 extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
 extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
-extern bool vlan_do_receive(struct sk_buff **skb);
+extern bool vlan_do_receive(struct sk_buff **skb, bool last_handler);
 extern struct sk_buff *vlan_untag(struct sk_buff *skb);
 
 #else
@@ -128,9 +128,9 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
 	return 0;
 }
 
-static inline bool vlan_do_receive(struct sk_buff **skb)
+static inline bool vlan_do_receive(struct sk_buff **skb, bool last_handler)
 {
-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && last_handler)
 		(*skb)->pkt_type = PACKET_OTHERHOST;
 	return false;
 }
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f1f2f7b..163397f 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -4,7 +4,7 @@
 #include <linux/netpoll.h>
 #include "vlan.h"
 
-bool vlan_do_receive(struct sk_buff **skbp)
+bool vlan_do_receive(struct sk_buff **skbp, bool last_handler)
 {
 	struct sk_buff *skb = *skbp;
 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
 
 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
 	if (!vlan_dev) {
-		if (vlan_id)
+		/* Only the last call to vlan_do_receive() should change
+		 * pkt_type to PACKET_OTHERHOST
+		 */
+		if (vlan_id && last_handler)
 			skb->pkt_type = PACKET_OTHERHOST;
 		return false;
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index edcf019..6ba50a1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3283,18 +3283,18 @@ another_round:
 ncls:
 #endif
 
+	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (vlan_tx_tag_present(skb)) {
 		if (pt_prev) {
 			ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = NULL;
 		}
-		if (vlan_do_receive(&skb))
+		if (vlan_do_receive(&skb, !rx_handler))
 			goto another_round;
 		else if (unlikely(!skb))
 			goto out;
 	}
 
-	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (rx_handler) {
 		if (pt_prev) {
 			ret = deliver_skb(skb, pt_prev, orig_dev);

Re: [PATCH v2] vlan: allow nested vlan_do_receive()

[Jiri Pirko]

Sat, Oct 29, 2011 at 06:13:39PM CEST, eric.dumazet@gmail.com wrote:
>commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>broke ARP processing on vlan on top of bonding.
>
>       +-------+
>eth0 --| bond0 |---bond0.103
>eth1 --|       |
>       +-------+
>
>52870.115435: skb_gro_reset_offset <-napi_gro_receive
>52870.115435: dev_gro_receive <-napi_gro_receive
>52870.115435: napi_skb_finish <-napi_gro_receive
>52870.115435: netif_receive_skb <-napi_skb_finish
>52870.115435: get_rps_cpu <-netif_receive_skb
>52870.115435: __netif_receive_skb <-netif_receive_skb
>52870.115436: vlan_do_receive <-__netif_receive_skb
>52870.115436: bond_handle_frame <-__netif_receive_skb
>52870.115436: vlan_do_receive <-__netif_receive_skb
>52870.115436: arp_rcv <-__netif_receive_skb
>52870.115436: kfree_skb <-arp_rcv
>
>Packet is dropped in arp_rcv() because its pkt_type was set to
>PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>exists.
>
>We really need to change pkt_type only if no more rx_handler is about to
>be called for the packet.
>
>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>---
>V2 : change the vlan_do_receive() added argument to be a boolean
>
> include/linux/if_vlan.h |    6 +++---
> net/8021q/vlan_core.c   |    7 +++++--
> net/core/dev.c          |    4 ++--
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
>diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>index 44da482..12d5543 100644
>--- a/include/linux/if_vlan.h
>+++ b/include/linux/if_vlan.h
>@@ -106,7 +106,7 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
> extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
> extern u16 vlan_dev_vlan_id(const struct net_device *dev);
> 
>-extern bool vlan_do_receive(struct sk_buff **skb);
>+extern bool vlan_do_receive(struct sk_buff **skb, bool last_handler);
> extern struct sk_buff *vlan_untag(struct sk_buff *skb);
> 
> #else
>@@ -128,9 +128,9 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
> 	return 0;
> }
> 
>-static inline bool vlan_do_receive(struct sk_buff **skb)
>+static inline bool vlan_do_receive(struct sk_buff **skb, bool last_handler)
> {
>-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && last_handler)
> 		(*skb)->pkt_type = PACKET_OTHERHOST;
> 	return false;
> }
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index f1f2f7b..163397f 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -4,7 +4,7 @@
> #include <linux/netpoll.h>
> #include "vlan.h"
> 
>-bool vlan_do_receive(struct sk_buff **skbp)
>+bool vlan_do_receive(struct sk_buff **skbp, bool last_handler)
> {
> 	struct sk_buff *skb = *skbp;
> 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
> 
> 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
> 	if (!vlan_dev) {
>-		if (vlan_id)
>+		/* Only the last call to vlan_do_receive() should change
>+		 * pkt_type to PACKET_OTHERHOST
>+		 */
>+		if (vlan_id && last_handler)
> 			skb->pkt_type = PACKET_OTHERHOST;
> 		return false;
> 	}
>diff --git a/net/core/dev.c b/net/core/dev.c
>index edcf019..6ba50a1 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -3283,18 +3283,18 @@ another_round:
> ncls:
> #endif
> 
>+	rx_handler = rcu_dereference(skb->dev->rx_handler);
> 	if (vlan_tx_tag_present(skb)) {
> 		if (pt_prev) {
> 			ret = deliver_skb(skb, pt_prev, orig_dev);
> 			pt_prev = NULL;
> 		}
>-		if (vlan_do_receive(&skb))
>+		if (vlan_do_receive(&skb, !rx_handler))

This I had on mind as well. Looks nicer. I have one another thought how
to resolve this. I will try it and let you know by tomorrow.

Jirka

> 			goto another_round;
> 		else if (unlikely(!skb))
> 			goto out;
> 	}
> 
>-	rx_handler = rcu_dereference(skb->dev->rx_handler);
> 	if (rx_handler) {
> 		if (pt_prev) {
> 			ret = deliver_skb(skb, pt_prev, orig_dev);
>
>

Re: [PATCH v2] vlan: allow nested vlan_do_receive()

[Jiri Pirko]

Sat, Oct 29, 2011 at 06:28:40PM CEST, jpirko@redhat.com wrote:
>Sat, Oct 29, 2011 at 06:13:39PM CEST, eric.dumazet@gmail.com wrote:
>>commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>>broke ARP processing on vlan on top of bonding.
>>
>>       +-------+
>>eth0 --| bond0 |---bond0.103
>>eth1 --|       |
>>       +-------+
>>
>>52870.115435: skb_gro_reset_offset <-napi_gro_receive
>>52870.115435: dev_gro_receive <-napi_gro_receive
>>52870.115435: napi_skb_finish <-napi_gro_receive
>>52870.115435: netif_receive_skb <-napi_skb_finish
>>52870.115435: get_rps_cpu <-netif_receive_skb
>>52870.115435: __netif_receive_skb <-netif_receive_skb
>>52870.115436: vlan_do_receive <-__netif_receive_skb
>>52870.115436: bond_handle_frame <-__netif_receive_skb
>>52870.115436: vlan_do_receive <-__netif_receive_skb
>>52870.115436: arp_rcv <-__netif_receive_skb
>>52870.115436: kfree_skb <-arp_rcv
>>
>>Packet is dropped in arp_rcv() because its pkt_type was set to
>>PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>>exists.
>>
>>We really need to change pkt_type only if no more rx_handler is about to
>>be called for the packet.
>>
>>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>>---
>>V2 : change the vlan_do_receive() added argument to be a boolean
>>
>> include/linux/if_vlan.h |    6 +++---
>> net/8021q/vlan_core.c   |    7 +++++--
>> net/core/dev.c          |    4 ++--
>> 3 files changed, 10 insertions(+), 7 deletions(-)
>>
>>diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>>index 44da482..12d5543 100644
>>--- a/include/linux/if_vlan.h
>>+++ b/include/linux/if_vlan.h
>>@@ -106,7 +106,7 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
>> extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
>> extern u16 vlan_dev_vlan_id(const struct net_device *dev);
>> 
>>-extern bool vlan_do_receive(struct sk_buff **skb);
>>+extern bool vlan_do_receive(struct sk_buff **skb, bool last_handler);
>> extern struct sk_buff *vlan_untag(struct sk_buff *skb);
>> 
>> #else
>>@@ -128,9 +128,9 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
>> 	return 0;
>> }
>> 
>>-static inline bool vlan_do_receive(struct sk_buff **skb)
>>+static inline bool vlan_do_receive(struct sk_buff **skb, bool last_handler)
>> {
>>-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>>+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && last_handler)
>> 		(*skb)->pkt_type = PACKET_OTHERHOST;
>> 	return false;
>> }
>>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>index f1f2f7b..163397f 100644
>>--- a/net/8021q/vlan_core.c
>>+++ b/net/8021q/vlan_core.c
>>@@ -4,7 +4,7 @@
>> #include <linux/netpoll.h>
>> #include "vlan.h"
>> 
>>-bool vlan_do_receive(struct sk_buff **skbp)
>>+bool vlan_do_receive(struct sk_buff **skbp, bool last_handler)
>> {
>> 	struct sk_buff *skb = *skbp;
>> 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>>@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
>> 
>> 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
>> 	if (!vlan_dev) {
>>-		if (vlan_id)
>>+		/* Only the last call to vlan_do_receive() should change
>>+		 * pkt_type to PACKET_OTHERHOST
>>+		 */
>>+		if (vlan_id && last_handler)
>> 			skb->pkt_type = PACKET_OTHERHOST;
>> 		return false;
>> 	}
>>diff --git a/net/core/dev.c b/net/core/dev.c
>>index edcf019..6ba50a1 100644
>>--- a/net/core/dev.c
>>+++ b/net/core/dev.c
>>@@ -3283,18 +3283,18 @@ another_round:
>> ncls:
>> #endif
>> 
>>+	rx_handler = rcu_dereference(skb->dev->rx_handler);
>> 	if (vlan_tx_tag_present(skb)) {
>> 		if (pt_prev) {
>> 			ret = deliver_skb(skb, pt_prev, orig_dev);
>> 			pt_prev = NULL;
>> 		}
>>-		if (vlan_do_receive(&skb))
>>+		if (vlan_do_receive(&skb, !rx_handler))
>
>This I had on mind as well. Looks nicer. I have one another thought how
>to resolve this. I will try it and let you know by tomorrow.

Okay that would not work.

So I'm okay with this patch 


Reviewed-by: Jiri Pirko <jpirko@redhat.com>


>
>Jirka
>
>> 			goto another_round;
>> 		else if (unlikely(!skb))
>> 			goto out;
>> 	}
>> 
>>-	rx_handler = rcu_dereference(skb->dev->rx_handler);
>> 	if (rx_handler) {
>> 		if (pt_prev) {
>> 			ret = deliver_skb(skb, pt_prev, orig_dev);
>>
>>

Re: [PATCH v2] vlan: allow nested vlan_do_receive()

[David Miller]

From: Jiri Pirko <jpirko@redhat.com>
Date: Sun, 30 Oct 2011 09:38:12 +0100

> Sat, Oct 29, 2011 at 06:28:40PM CEST, jpirko@redhat.com wrote:
>>Sat, Oct 29, 2011 at 06:13:39PM CEST, eric.dumazet@gmail.com wrote:
>>>commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>>>broke ARP processing on vlan on top of bonding.
 ...
>>>Packet is dropped in arp_rcv() because its pkt_type was set to
>>>PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>>>exists.
>>>
>>>We really need to change pkt_type only if no more rx_handler is about to
>>>be called for the packet.
>>>
>>>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>>>---
>>>V2 : change the vlan_do_receive() added argument to be a boolean
 ...
> Reviewed-by: Jiri Pirko <jpirko@redhat.com>

Applied, thanks everyone.

Re: [PATCH v2] vlan: allow nested vlan_do_receive()

[Eric Dumazet]

Le dimanche 30 octobre 2011 à 09:38 +0100, Jiri Pirko a écrit :


> So I'm okay with this patch 
> 
> 
> Reviewed-by: Jiri Pirko <jpirko@redhat.com>
> 

Thanks a lot for this review

Re: [net-next PATCH] net: allow vlan traffic to be received under bond

[Jiri Pirko]

Sat, Oct 29, 2011 at 06:00:26PM CEST, eric.dumazet@gmail.com wrote:
>Le samedi 29 octobre 2011 à 16:59 +0200, Jiri Pirko a écrit :
>> Sat, Oct 29, 2011 at 12:22:26PM CEST, eric.dumazet@gmail.com wrote:
>> >Le vendredi 28 octobre 2011 à 19:20 -0700, John Fastabend a écrit :
>> >
>> >> Thanks Eric! Thought about this some and I haven't come up
>> >> with anything better yet. Even though this might be a slight
>> >> hack I would prefer this to reverting the patch.
>> >> 
>> >> I'll think about this more tomorrow. Would you be against
>> >> submitting this patch?
>> >
>> >I cant submit this patch, because its a hack and partial fix.
>> >
>> >For Unicast packets, we still do the wrong thing : setting their
>> >pkt_type to PACKET_OTHERHOST before the call to rx_handler :
>> >
>> >In this case, bond_handle_frame() wont handle this packet correctly in
>> >some cases (BOND_MODE_ALB ...). I suppose bridge might be confused as
>> >well. So other problems remain.
>> >
>> >We should delay the PACKET_OTHERHOST setting to the last moment, that is
>> >the last time vlan_do_receive() is called.
>> >
>> >What about following patch instead ?
>> >
>> >[PATCH] vlan: allow nested vlan_do_receive()
>> >
>> >commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>> >broke ARP processing on vlan on top of bonding.
>> >
>> >       +-------+
>> >eth0 --| bond0 |---bond0.103
>> >eth1 --|       |
>> >       +-------+
>> >
>> >52870.115435: skb_gro_reset_offset <-napi_gro_receive
>> >52870.115435: dev_gro_receive <-napi_gro_receive
>> >52870.115435: napi_skb_finish <-napi_gro_receive
>> >52870.115435: netif_receive_skb <-napi_skb_finish
>> >52870.115435: get_rps_cpu <-netif_receive_skb
>> >52870.115435: __netif_receive_skb <-netif_receive_skb
>> >52870.115436: vlan_do_receive <-__netif_receive_skb
>> >52870.115436: bond_handle_frame <-__netif_receive_skb
>> >52870.115436: vlan_do_receive <-__netif_receive_skb
>> >52870.115436: arp_rcv <-__netif_receive_skb
>> >52870.115436: kfree_skb <-arp_rcv
>> >
>> >Packet is dropped in arp_rcv() because its pkt_type was set to
>> >PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>> >exists.
>> >
>> >We really need to change pkt_type only if no more rx_handler is about to
>> >be called for the packet.
>> >
>> >Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> >---
>> > include/linux/if_vlan.h |    8 +++++---
>> > net/8021q/vlan_core.c   |    7 +++++--
>> > net/core/dev.c          |    4 ++--
>> > 3 files changed, 12 insertions(+), 7 deletions(-)
>> >
>> >diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>> >index 44da482..95874ff 100644
>> >--- a/include/linux/if_vlan.h
>> >+++ b/include/linux/if_vlan.h
>> >@@ -106,7 +106,8 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
>> > extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
>> > extern u16 vlan_dev_vlan_id(const struct net_device *dev);
>> > 
>> >-extern bool vlan_do_receive(struct sk_buff **skb);
>> >+extern bool vlan_do_receive(struct sk_buff **skb,
>> >+			    rx_handler_func_t *rx_handler);
>> > extern struct sk_buff *vlan_untag(struct sk_buff *skb);
>> > 
>> > #else
>> >@@ -128,9 +129,10 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
>> > 	return 0;
>> > }
>> > 
>> >-static inline bool vlan_do_receive(struct sk_buff **skb)
>> >+static inline bool vlan_do_receive(struct sk_buff **skb,
>> >+				   rx_handler_func_t *rx_handler)
>> > {
>> >-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>> >+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && !rx_handler)
>> > 		(*skb)->pkt_type = PACKET_OTHERHOST;
>> > 	return false;
>> > }
>> >diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> >index f1f2f7b..3ec1ada 100644
>> >--- a/net/8021q/vlan_core.c
>> >+++ b/net/8021q/vlan_core.c
>> >@@ -4,7 +4,7 @@
>> > #include <linux/netpoll.h>
>> > #include "vlan.h"
>> > 
>> >-bool vlan_do_receive(struct sk_buff **skbp)
>> >+bool vlan_do_receive(struct sk_buff **skbp, rx_handler_func_t *rx_handler)
>> > {
>> > 	struct sk_buff *skb = *skbp;
>> > 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>> >@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
>> > 
>> > 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
>> > 	if (!vlan_dev) {
>> >-		if (vlan_id)
>> >+		/* Only the last call to vlan_do_receive() should change
>> >+		 * pkt_type to PACKET_OTHERHOST
>> >+		 */
>> >+		if (vlan_id && !rx_handler)
>> > 			skb->pkt_type = PACKET_OTHERHOST;
>> > 		return false;
>> > 	}
>> >diff --git a/net/core/dev.c b/net/core/dev.c
>> >index edcf019..40976b4 100644
>> >--- a/net/core/dev.c
>> >+++ b/net/core/dev.c
>> >@@ -3283,18 +3283,18 @@ another_round:
>> > ncls:
>> > #endif
>> > 
>> >+	rx_handler = rcu_dereference(skb->dev->rx_handler);
>> > 	if (vlan_tx_tag_present(skb)) {
>> > 		if (pt_prev) {
>> > 			ret = deliver_skb(skb, pt_prev, orig_dev);
>> > 			pt_prev = NULL;
>> > 		}
>> >-		if (vlan_do_receive(&skb))
>> >+		if (vlan_do_receive(&skb, rx_handler))
>> 
>> I must say I do not like passing rx_handler out like this. Apart it's
>> not nice, it might be misleading....
>> 
>> How about something like following instead? I must test it but I believe
>> it should resolve the problem.
>> 
>> 
>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>> index 44da482..165a487 100644
>> --- a/include/linux/if_vlan.h
>> +++ b/include/linux/if_vlan.h
>> @@ -130,8 +130,6 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
>>  
>>  static inline bool vlan_do_receive(struct sk_buff **skb)
>>  {
>> -	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>> -		(*skb)->pkt_type = PACKET_OTHERHOST;
>>  	return false;
>>  }
>>  
>> @@ -141,6 +139,14 @@ static inline struct sk_buff *vlan_untag(struct sk_buff *skb)
>>  }
>>  #endif
>>  
>> +static inline void vlan_handle_leftover(struct sk_buff *skb)
>> +{
>> +	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>> +
>> +	if (vlan_id)
>> +		skb->pkt_type = PACKET_OTHERHOST;
>> +}
>> +
>>  /**
>>   * vlan_insert_tag - regular VLAN tag inserting
>>   * @skb: skbuff to tag
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index f1f2f7b..540da12 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -12,11 +12,8 @@ bool vlan_do_receive(struct sk_buff **skbp)
>>  	struct vlan_pcpu_stats *rx_stats;
>>  
>>  	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
>> -	if (!vlan_dev) {
>> -		if (vlan_id)
>> -			skb->pkt_type = PACKET_OTHERHOST;
>> +	if (!vlan_dev)
>>  		return false;
>> -	}
>>  
>>  	skb = *skbp = skb_share_check(skb, GFP_ATOMIC);
>>  	if (unlikely(!skb))
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index b7ba81a..6fdfcc9 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3314,6 +3314,14 @@ ncls:
>>  		}
>>  	}
>>  
>> +	if (vlan_tx_tag_present(skb)) {
>> +		/*
>> +		 * Tag is still present here. That means there's no device
>> +		 * set up for this vlan id. So handle these leftovers here.
>> +		 */
>> +		vlan_handle_leftover(skb);
>> +	}
>> +
>>  	/* deliver only exact match when indicated */
>>  	null_or_dev = deliver_exact ? skb->dev : NULL;
>>  
>
>Hmm, is it really working ? where vlan_tci is cleared ?

Near the end of vlan_do_receive.

>
>This is indeed nice but adds another test in fast path, while in my
>patch, additional tests are done in slow path only.

Oh, vlan_tx_tag_present() check adds overhead that can be waived upon I
suppose. I think it's better to be nice here...

>
>I see nothing wrong with passing rx_handler : Its probably cleaner than
>adding rcu_dereference_raw(skb->dev->rx_handler) in the two
>vlan_do_receive() implementations...

Cleaner for sure.

>
>For reference, this was the first patch I had in mind, before I decided
>that caller had to pass the rx_handler instead. (It could be a boolean
>instead)
>
> include/linux/if_vlan.h |    3 ++-
> net/8021q/vlan_core.c   |    5 ++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>index 44da482..c6c00b4 100644
>--- a/include/linux/if_vlan.h
>+++ b/include/linux/if_vlan.h
>@@ -130,7 +130,8 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
> 
> static inline bool vlan_do_receive(struct sk_buff **skb)
> {
>-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>+	if (((*skb)->vlan_tci & VLAN_VID_MASK) &&
>+	    !rcu_dereference_raw((*skb)->dev->rx_handler))
> 		(*skb)->pkt_type = PACKET_OTHERHOST;
> 	return false;
> }
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index f1f2f7b..245efb8 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
> 
> 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
> 	if (!vlan_dev) {
>-		if (vlan_id)
>+		/* Only the last call to vlan_do_receive() should change
>+		 * pkt_type to PACKET_OTHERHOST
>+		 */
>+		if (vlan_id && !rcu_dereference_raw(skb->dev->rx_handler))
> 			skb->pkt_type = PACKET_OTHERHOST;
> 		return false;
> 	}
>
>