From: Hans Schillstrom <hans.schillstrom@ericsson•com>
To: Jesse Gross <jesse@nicira•com>
Cc: John Fastabend <john.r.fastabend@intel•com>,
Jiri Pirko <jpirko@redhat•com>,
"davem@davemloft•net" <davem@davemloft•net>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"fubar@us•ibm.com" <fubar@us•ibm.com>
Subject: Re: [net-next PATCH] net: allow vlan traffic to be received under bond
Date: Tue, 11 Oct 2011 13:08:51 +0200 [thread overview]
Message-ID: <201110111308.53152.hans.schillstrom@ericsson.com> (raw)
In-Reply-To: <CAEP_g=83tCQMVPOTT82GBw6GamodpqU-SAeYyx9noWnzbGfUpg@mail.gmail.com>
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>
next prev parent reply other threads:[~2011-10-11 11:08 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-10-10 19:16 [net-next PATCH] net: allow vlan traffic to be received under bond John Fastabend
2011-10-10 22:37 ` Jiri Pirko
2011-10-11 2:07 ` John Fastabend
2011-10-11 2:43 ` Jesse Gross
2011-10-11 11:08 ` Hans Schillstrom [this message]
2011-10-11 13:13 ` John Fastabend
2011-10-13 13:09 ` Hans Schillström
2011-10-11 13:16 ` John Fastabend
2011-10-11 10:57 ` Jiri Pirko
2011-10-13 15:04 ` Maxime Bizon
2011-10-13 15:38 ` Jiri Pirko
2011-10-13 15:48 ` Maxime Bizon
2011-10-13 15:59 ` Hans Schillström
2011-10-13 17:42 ` John Fastabend
2011-10-13 18:23 ` Hans Schillström
2011-10-14 0:22 ` Jesse Gross
2011-10-19 3:47 ` David Miller
2011-10-28 10:00 ` Eric Dumazet
2011-10-28 11:06 ` Eric Dumazet
2011-10-29 2:20 ` John Fastabend
2011-10-29 10:22 ` Eric Dumazet
2011-10-29 14:59 ` Jiri Pirko
2011-10-29 16:00 ` Eric Dumazet
2011-10-29 16:13 ` [PATCH v2] vlan: allow nested vlan_do_receive() Eric Dumazet
2011-10-29 16:28 ` Jiri Pirko
2011-10-30 8:38 ` Jiri Pirko
2011-10-30 8:44 ` David Miller
2011-10-30 8:44 ` Eric Dumazet
2011-10-29 16:16 ` [net-next PATCH] net: allow vlan traffic to be received under bond Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201110111308.53152.hans.schillstrom@ericsson.com \
--to=hans.schillstrom@ericsson$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=fubar@us$(echo .)ibm.com \
--cc=jesse@nicira$(echo .)com \
--cc=john.r.fastabend@intel$(echo .)com \
--cc=jpirko@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox