On Mon, Jul 18, 2016 at 11:21:11PM +0000, Jan Scheurich wrote: > Hi Johnson, > > > From: dev [mailto:dev-boun...@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...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
