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

[Jesse Gross]

On Tue, Mar 12, 2013 at 12:26 AM, Simon Horman <horms at verge.net.au> wrote:
> On Mon, Mar 11, 2013 at 10:00:54AM -0700, Jesse Gross wrote:
>> On Mon, Mar 11, 2013 at 6:02 AM, Rajahalme, Jarno (NSN - FI/Espoo)
>> <jarno.rajahalme at nsn.com> wrote:
>> >
>> > On Mar 9, 2013, at 16:54 , ext Simon Horman wrote:
>> >
>> >> It is not obvious to me how the behaviour relating to get_priority() as
>> >> described above can sensibly be reconciled with a desire to preserve the
>> >> order of actions. Or even reconciled with a looser constraint that actions
>> >> that need l3+ data occur before push_mpls actions.
>> >
>> > Maybe do_xlate_actions() should take care of translating the skb_priority to nw_tos, and make sure that it happens before any L2.5 actions are translated. This would make any later set actions on skb_priority ineffective, though. Also, it seems that on the patch port output the nw_tos is not set, so that might require some further thought. Maybe it is of no functional use for the network transport, but still the peer might actually depend (via a match) on the nw_tos being set the same way as any other port might have it?
>>
>> I agree that simply moving the setting of nw_tos earlier in the
>> process is the best way to handle this.  A similar problem occurs if
>> an MPLS (or any other non-IP) packet is received on the wire -
>> changing the priority will have no effect on the marking.  Since
>> pushing MPLS labels and setting the priority are both done by the
>> controller, this behavior should at least be fairly transparent and
>> not surprising.
>
> Although it seems like an invasive change I can see how it may be possible
> for ovs-vwitchd to clone the packet and emit multiple execute operations, one
> per output port.
>
> However, I'm not sure I see how an appropriate put operation can be emitted
> to the datapath using this scheme as, recirculation aside, there can only
> be one facet per match.
>
> I'm also a little dubious about the complexity of this change vs the
> objective of not adding an element to the skb callback.

No, I don't think that we should add a clone action.  I think the
overriding principle should be that any actions after an MPLS header
has been pushed should be the same as if a packet came in with an MPLS
header directly.