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

On Fri, Oct 04, 2013 at 05:09:56PM +0900, Simon Horman wrote:

From: Joe Stringer <redacted>

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 <redacted>
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.