[ovs-dev] [PATCH 2/2] datapath: Add basic MPLS support to kernel

[Jesse Gross]

On Wed, Mar 20, 2013 at 5:33 PM, Simon Horman <horms at verge.net.au> wrote:
> On Wed, Mar 20, 2013 at 03:43:38PM -0700, Jesse Gross wrote:
>> On Wed, Mar 20, 2013 at 6:18 AM, Simon Horman <horms at verge.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 and Ravi K.
>> >
>> > Cc: Ravi K <rkerur at gmail.com>
>> > Cc: Leo Alterman <lalterman at nicira.com>
>> > Reviewed-by: Isaku Yamahata <yamahata at valinux.co.jp>
>> > Signed-off-by: Simon Horman <horms at verge.net.au>
>> >
>> > ---
>> >
>> > TODO:
>> > * Enhance core kernel code to handle GSO for MPLS or
>> >   otherwise deal with accelerations. (Linux network core)
>> > * Add ETH_TYPE_MIN or similar to Linux network core
>> >
>> > v2.22
>> > * As suggested by Jesse Gross:
>> >   - Fix sparse warning in validate_and_copy_actions()
>> >     I have no idea why sparse doesn't show this up this on my system.
>> >   - Remove call to skb_cow_head() from push_mpls() as it
>> >     is already covered by a call to make_writable()
>> >   - Check (key_type > OVS_KEY_ATTR_MAX) in ovs_flow_verify_key_len()
>> >   - Disallow set actions on l2.5+ data and MPLS push and pop actions
>> >     after an MPLS pop action as there is no verification that the packet
>> >     is actually of the new ethernet type. This may later be supported
>> >     using recirculation or by other means.
>> >   - Do not add spurious debuging message to ovs_flow_cmd_new_or_set()
>>
>> There were a couple other things from the previous time that don't
>> seem to have made it here.  Looking back I see:
>>  * MPLS label userspace/kernel change (just the type definition) and
>> only allowing a single label in the stack at this time.
>
> This one I was planning to fix but it slipped my mind.
>
>>  * Validation of actions using both paths of the sample action.
>
> This one I had not realised was related to MPLS.
> Could you explain it in a little more detail?

Currently, no action can affect the validity of any other action, so
we just evaluate sample actions unconditionally to make sure that we
hit all of them.  However, with MPLS that's no longer the case - a
push_mpls action can make a set(mpls) action valid or a set(ip) action
invalid.

Right now, we're not carrying the current_eth_type through the sample
action into/out of it's subactions.  I think we need to do that and
also evaluate both paths to make sure that they are each valid.