Message-ID: <20130712015538.GA30897@verge.net.au>
Date:	Fri, 12 Jul 2013 10:55:38 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Jesse Gross <jesse@...ira.com>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>, Ravi K <rkerur@...il.com>,
	Isaku Yamahata <yamahata@...inux.co.jp>,
	Pravin B Shelar <pshelar@...ira.com>,
	Jarno Rajahalme <jarno.rajahalme@....com>,
	Joe Stringer <joe@...d.net.nz>
Subject: Re: [PATCH v2.34] datapath: Add basic MPLS support to kernel

On Thu, Jul 11, 2013 at 06:26:32PM -0700, Jesse Gross wrote:
> On Wed, Jul 10, 2013 at 5:16 PM, Simon Horman <horms@...ge.net.au> wrote:
> > On Mon, Jul 08, 2013 at 11:51:18PM -0700, Jesse Gross wrote:
> >> On Tue, Jul 2, 2013 at 6:31 PM, Simon Horman <horms@...ge.net.au> wrote:
> >> > On Tue, Jul 02, 2013 at 02:59:51PM -0700, Jesse Gross wrote:
> >> >> On Mon, Jul 1, 2013 at 9:16 PM, Simon Horman <horms@...ge.net.au> wrote:
> >> >> > On Mon, Jul 01, 2013 at 04:42:46PM -0700, Jesse Gross wrote:
> >> >> >> On Wed, Jun 26, 2013 at 1:18 AM, Simon Horman <horms@...ge.net.au> wrote:
> >> >> >> > Allow datapath to recognize and extract MPLS labels into flow keys
> >> >> >> > and execute actions which push, pop, and set labels on packets.
> >> >> >> >
> >> >> >> > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
> >> >> >> >
> >> >> >> > Cc: Ravi K <rkerur@...il.com>
> >> >> >> > Cc: Leo Alterman <lalterman@...ira.com>
> >> >> >> > Cc: Isaku Yamahata <yamahata@...inux.co.jp>
> >> >> >> > Cc: Joe Stringer <joe@...d.net.nz>
> >> >> >> > Signed-off-by: Simon Horman <horms@...ge.net.au>
> >> >> >>
> >> >> >> Simon, have you thought any more about the header ordering issues? I
> >> >> >> don't think we've reached a conclusion at this point.
> >> >> >
> >> >> > I believe that you then raised the issue of QinQ, which somehow
> >> >> > I failed to respond to, I apologise for that.
> >> >> >
> >> >> > In particular, my understanding is that you are concerned the code will
> >> >> > miss-calculate the end of L2 headers in the presence of multiple VLAN tags.
> >> >> > Thus resulting in an MPLS push action inserting an MPLS LSE after the first
> >> >> > rather than the last VLAN tag. And that would likely change if QinQ support
> >> >> > was added to Open vSwtich.
> >> >> >
> >> >> > I wonder if a good way forwards is to enhance the calculation
> >> >> > of the end of L2 headers (mac_len) and the beginning of L3 headers
> >> >> > (network_header) in ovs_flow_extract() such that it takes into
> >> >> > account the presence of more than one VLAN tag.
> >> >>
> >> >> The problem is that this requires being able to calculate the length
> >> >> of all possible headers that we might want to support in the future.
> >> >> In the case of QinQ, this would mean the various EtherTypes. You could
> >> >> also imagine other things like MAC-in-MAC that are farther afield from
> >> >> what we currently support.
> >> >
> >> > That is true.
> >> >
> >> > I think that the key problem is it that it is hard for us
> >> > to correctly determine where the end of the L2 header is,
> >> > or more specifically where the MPLS tag should go, for all cases.
> >> > Particularly cases which are yet to be defined.
> >> >
> >> > Having spoken with Joe about this we see two main options:
> >> >
> >> > 1. The status quo as of this patch. Which is that MPLS actions
> >> >    may be invalid for some cases.
> >> >
> >> >    While it should be be possible to solve individual cases,
> >> >    this doesn't solve the general case.
> >> >
> >> > 2. Only allow MPLS actions on ether types where the implementation
> >> >    is known to work.
> >> >
> >> >    This could act as a white list of sorts. It could start with
> >> >    the obvious candidates: IPv4, IPv6, ARP, 802.1Q,...
> >> >    And support for more protocols could be added in the future.
> >> >
> >> >    This would seem to reflect the somewhat special nature of MPLS.
> >>
> >> I think what is really necessary at the kernel level is some
> >> flexibility about where to put the newly inserted MPLS header. Then
> >> you could basically chose either of the two options above or export
> >> the flexibility through OpenFlow (which by my reading of the spec is
> >> already supposed to be allowed). Furthermore, you could do different
> >> things in different situations as OpenFlow evolves, which really has
> >> to be done at the userspace level since only it has the full set of
> >> knowledge about the protocol.
> >
> > I wonder if this can be achieved by adding a list of features to
> > the MPLS push action, for example as a possibly zero-length array of
> > integers which encode feature bits.
> >
> > At the time that MPLS support is added to the datapath we could define that
> > all the bits are zero and the behaviour of the code at that time is the
> > expected behaviour.
> >
> > Suppose that a new feature is added in the future.  I will use the example
> > of support for 802.1ad (the standardised variant of Q-in-Q).
> >
> > The logic in the datapath to determine the end of the L2 header and thus
> > the top of the MPLS LSE stack could be guarded by a new feature bit,
> > the ad-bit.
> >
> > If an MPLS push action, supplied by userspace, has the ad-bit set then the
> > new logic is used and the MPLS LSE is inserted accordingly.
> > Conversely, if the MPLS push action does not have the bit set then the
> > old logic is used and the MPLS LSE is inserted as if the datapath
> > didn't understand 802.1ad.
> >
> > In this way it would be possible for userspace to select the desired
> > behaviour. An old user-space would use the old behaviour. A new userspace
> > may choose the old or the new behaviour.
> >
> > And it would be possible for the datapath to reject facets with MPLS
> > push actions with feature bits or combinations of feature bits that
> > are not supported.
> 
> Hmm, I think that this may become fairly complicated over time as you
> have a number of different types.
> 
> Going back to the OpenFlow spec:
> 
> "Newly pushed tags should always be inserted as the outermost tag in
> the outermost valid location for
> that tag. When a new VLAN tag is pushed, it should be the outermost
> tag inserted, immediately after
> the Ethernet header and before other tags. Likewise, when a new MPLS
> tag is pushed, it should be the
> outermost tag inserted, immediately after the Ethernet header and
> before other tags.
> 
> When multiple push actions are added to the action set of the packet,
> they apply to the packet in the
> order defined by the action set rules, first MPLS, then PBB, than VLAN
> (se 5.10). When multiple push
> actions are included in an action list, they apply to the packet in
> the list order (see 5.11)."
> 
> This seems about as flexible as anything that I can think of at the
> moment and fairly straightforward: basically we wouldn't need to skip
> over vlan tags at the beginning because we would just push tags in
> front of them. If userspace wants them in the opposite order then it
> can pop off the tags and put them back but I suspect that this is
> actually quite uncommon.

Thanks.

I agree that your proposed scheme should cover all the bases.

In the case of OpenFlow 1.2 I think the spec is fairly clear that the MPLS
label stack should follow any VLAN tags. So I now plan to add logic to pop
off the VLAN tags and put them on, as you suggest above.

> I know you mentioned before that the valid location for the MPLS label
> is after the vlan tags but there are several ways to use MPLS and I
> think the last line of the first paragraph is fairly clear.

With that in mind I plan to have userspace use the order that you suggest,
just adding the MPLS as the outer-most tag, in the case of OpenFlow 1.3.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists