On Fri, Oct 04, 2013 at 05:09:56PM +0900, Simon Horman wrote: > From: Joe Stringer <[email protected]> > > OpenFlow 1.1 and 1.2, and 1.3 differ in their handling of MPLS actions in the > presence of VLAN tags. To allow correct behaviour to be committed in > each situation, this patch adds a second round of VLAN tag action > handling to commit_odp_actions(), which occurs after MPLS actions. This > is implemented with a new field in 'struct xlate_in' called 'vlan_tci'. > > When an push_mpls action is composed, the flow's current VLAN state is > stored into xin->vlan_tci, and flow->vlan_tci is set to 0 (pop_vlan). If > a VLAN tag is present, it is stripped; if not, then there is no change. > Any later modifications to the VLAN state is written to xin->vlan_tci. > When committing the actions, flow->vlan_tci is used before MPLS actions, > and xin->vlan_tci is used afterwards. This retains the current datapath > behaviour, but allows VLAN actions to be applied in a more flexible > manner. > > Both before and after this patch MPLS LSEs are pushed onto a packet after > any VLAN tags that may be present. This is the behaviour described in > OpenFlow 1.1 and 1.2. OpenFlow 1.3 specifies that MPLS LSEs should be > pushed onto a packet before any VLAN tags that are present. Support > for this will be added by a subsequent patch that makes use of > the infrastructure added by this patch. > > Signed-off-by: Joe Stringer <[email protected]> > Signed-off-by: Simon Horman <[email protected]>
I noticed a couple more minor points. First, it seems to me that the "vlan_tci" member that this adds to xlate_in could go in xlate_ctx just as well. I would prefer that, because (as far as I can tell) there is no reason for the client to use any value other than flow->vlan_tci here, and putting it in xlate_ctx hides it from the client. Thanks for rearranging the code and updating the comment in do_xlate_actions(). It makes the operation clearer. But now that it's clear I have an additional question. Does it really make sense to have 'vlan_tci' as only a local variable in do_xlate_actions()? Presumably, MPLS and VLANs should interact the same way regardless of whether they are separated by resubmits or goto_tables. That is, I suspect that this is xlate_ctx state, not local state. Thanks, Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
