[ovs-dev] [RFC PATCH v2 02/13] Format NSH keys to readable strings

[Simon Horman]

On Mon, Jul 18, 2016 at 11:21:11PM +0000, Jan Scheurich wrote:
> Hi Johnson,
> 
> > From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Yang, Yi
> > Sent: Monday, 18 July, 2016 12:37
> > 
> > On Mon, Jul 18, 2016 at 03:15:13PM +0900, Simon Horman wrote:
> > > Hi Johnson,
> > >
> > > On Wed, Jul 13, 2016 at 01:28:48AM +0800, Johnson Li wrote:
> > > > Signed-off-by: Johnson Li <johnson.li at intel.com>
> > >
> > > * Regarding the action set (which we discussed briefly off-list):
> > >
> > >   I think that you need to update ofpacts_execute_action_set(), though
> > >   possibly not in this patch, for push/pop_nsh to be usable in write
> > >   actions. However, its not clear to me at which position of that function
> > >   push/pop_nsh because as far as I know this has not been defined by
> > >   OpenFlow.
> 
> The OF standard specifies the following order for action set execution:
> 1. copy TTL inwards: apply copy TTL inward actions to the packet
> 2. pop: apply all tag pop actions to the packet
> 3. push-MPLS: apply MPLS tag push action to the packet
> 4. push-PBB: apply PBB tag push action to the packet
> 5. push-VLAN: apply VLAN tag push action to the packet
> 6. copy TTL outwards: apply copy TTL outwards action to the packet
> 7. decrement TTL: apply decrement TTL action to the packet
> 8. set: apply all set-field actions to the packet
> 9. qos: apply all QoS actions, such as set queue to the packet
> 10. group: if a group action is specified, apply the actions of the relevant group bucket(s) in the order specified by this list
> 11. output: if no group action is specified, forward the packet on the port specified by the output action
> 
> If both push_nsh and push_eth are in the action set, push_nsh should be executed before push_eth. Furthermore, if push_mpls and push_eth are present in the action set, push_eth should be executed before push_mpls (push_mpls pushes the MPLS shim header between MAC header and L2 payload, so the resulting encap would be Outer Ethernet, MPLS, Inner Ethernet, which may be useful for Ethernet over MPLS).
> 
> My suggestion for the action set execution order would therefore be:
> 3.a	push_nsh
> 3.b 	push_eth
> 3.c	push_mpls
> 4	push_pbb
> 5	push_vlan
> 
> I agree that this is somewhat arbitrary and can be debated, but one can achieve any wanted order using the Apply Actions instruction.

I think that sounds reasonable.

Actually, I think the important thing is to define the ordering clearly.
I wonder if taking it to the ONF earlier than later would be wise. It will
be painful if at some point they chose a different ordering to what is
present in the wild.

> > > * Regarding the implementation of push/pop_nsh in this patch:
> > >
> > >   In general translation occurs in two phases in OvS.
> > >
> > >   1. Composition: This generally involves updating fields of
> > >      ctx->xin->flow to new desired values and ctx->wc to indicate
> > >      which fields have been accessed so that masks for megaflows
> > >      can be calculated correctly.
> > >
> > >      For simple cases such as OFPACT_SET_IP_TTL this is open-coded
> > >      in do_xlate_actions. For more complex cases helpers are provided,
> > >      e.g.  OFPACT_PUSH_MPLS (though that is not very complex)
> > >
> > >   2. Commit: Here differences between ctx->in->flow and ctx->base_flow,
> > >      which are the same before translation starts, are compared. And any
> > >      differences are resolved by writing actions to ctx->odp_actions.
> > >      base_flow is then reset for cases where translation continues.
> > >
> > >      This is performed by commit_odp_actions(), e.g. when called via
> > >      xlate_commit_actions().
> > >
> > > There are exceptions to the above and in some cases actions are
> > > written directly to ctx->odp_actions, but I'm not sure that
> > > push/pop_nsh needs to be such an exception.
> 
> In addition to splitting up the push_nsh handling into compose and commit phase you need to consider the following:
> 
> As push_nsh pushes an encapsulation that changes the packet's ethertype and hides the inner packet, you need to call xlate_commit_actions(ctx) to really carry out all not yet committed actions on the inner packet before you start composing the push_nsh action.
> 
> Conversely, when you compose the pop_nsh action you need to trigger a recirculation of the packet after pop_nsh to have the datapath reparse the inner packet and subject it to another megaflow lookup. You should do that by calling  ctx_trigger_freeze(ctx) after having composed the pop_mpls action.
> 
> The same goes for the actions push_eth and pop_eth.

Yes, I believe that is true except for the last statement.

I think that push/pop_nsh will likely follow a similar pattern to
push/pop_mpls as both sets of actions may change the ethernet type of a
packet.  But I think the case of push/pop_eth is a little different. The
way this is modelled is that those actions affect the presence of an
ethernet header (which is in itself a bit special) but not the type of the
packet.