Re: [PATCH v2.42 1/5] odp: Allow VLAN actions after MPLS actions

[Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>]

On Fri, Oct 04, 2013 at 05:09:56PM +0900, Simon Horman wrote:
> From: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
> 
> 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 <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
> Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>

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.