Date:	Fri, 12 Jul 2013 09:22:33 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
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 6:55 PM, Simon Horman <horms@...ge.net.au> wrote:
> 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.

Great, sounds good to me.
--
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