From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Pfaff Subject: Re: [PATCH v2.40 2/7] odp: Allow VLAN actions after MPLS actions Date: Sun, 29 Sep 2013 10:05:49 -0700 Message-ID: <20130929170549.GC23836@nicira.com> References: <1380241116-7661-1-git-send-email-horms@verge.net.au> <1380241116-7661-3-git-send-email-horms@verge.net.au> <20130927192108.GA17506@nicira.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Simon Horman , "dev@openvswitch.org" , netdev@vger.kernel.org, Jesse Gross , Pravin B Shelar , Ravi K , Isaku Yamahata To: Joe Stringer Return-path: Received: from na3sys009aog134.obsmtp.com ([74.125.149.83]:41210 "HELO na3sys009aog134.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1754470Ab3I2RFz (ORCPT ); Sun, 29 Sep 2013 13:05:55 -0400 Received: by mail-pa0-f45.google.com with SMTP id rd3so4839126pab.18 for ; Sun, 29 Sep 2013 10:05:54 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Sep 29, 2013 at 02:51:19PM +1300, Joe Stringer wrote: > On Sat, Sep 28, 2013 at 7:21 AM, Ben Pfaff wrote: > > > On Fri, Sep 27, 2013 at 09:18:31AM +0900, Simon Horman wrote: > > > From: Joe Stringer > > > > > > OpenFlow 1.2 and 1.3 differ on 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. > > > > > > Signed-off-by: Joe Stringer > > > Signed-off-by: Simon Horman > > > > The commit message talks about handling OpenFlow 1.2 and 1.3 both > > properly, but I don't see how the code distinguishes between the cases. > > Can you explain? Maybe this is something added in a later patch, in > > which case it would be nice to mention that in the commit message. > > > > It is added in patch #5. IIRC this patch should cause no difference in > behaviour, but set up infrastructure to be used later. > > There seems to be a typo in the comment in vlan_tci_restore() here: > > > + /* If MPLS actions were executed after MPLS, copy the final > > vlan_tci out > > > + * and restore the intermediate VLAN state. */ > > > > Right, that should probably be "...executed after VLAN actions...". > > I was a little confused by the new local variable 'vlan_tci' in > > do_xlate_actions(). Partly this was because the fact that I didn't find > > it obvious that sometimes it points to different VLAN tags. I think > > this would be easier to see if it were initially assigned just under the > > big comment rather than in an initializer, so that one would know to > > look at the comment. > > > > Perhaps the big comment could be rearranged and put above the initialiser, > something like the following:- > > /* VLAN actions are stored to '*vlan_tci'. This variable initially points > to 'xin->flow->vlan_tci', so that > * VLAN actions are applied before any MPLS actions. When an MPLS action is > translated, > * 'vlan_tci' is updated to point to 'xin->vlan_tci'. This causes later > VLAN actions to be applied after MPLS actions. > * Each time through the loop, 'xin->vlan_tci' is updated to reflect the > final VLAN state of the flow. */ > > Then, the place where 'xin->vlan_tci' is updated to '*vlan_tci' could have > a simple comment to refer back:- > > /* Update the final vlan state to match the current state. */ All that makes sense, thanks.