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

On 10/10/2011 7:43 PM, Jesse Gross wrote:

On Mon, Oct 10, 2011 at 7:07 PM, John Fastabend
[off-list ref] wrote:

quoted

On 10/10/2011 3:37 PM, Jiri Pirko wrote:

quoted

Mon, Oct 10, 2011 at 09:16:41PM CEST, john.r.fastabend@intel.com wrote:

quoted

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 <redacted>
---

[...]

quoted

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 <redacted>

Thanks for the nice summary Jesse.