[ovs-dev] [PATCH] [RFC] flow: Do not clear L3+ fields of flow in flow_push_mpls()

Simon Horman horms at verge.net.au
Wed May 7 05:02:06 UTC 2014


On Thu, May 01, 2014 at 08:24:21AM -0700, Ben Pfaff wrote:
> On Thu, May 01, 2014 at 08:21:36AM -0700, Jarno Rajahalme wrote:
> > 
> > > On May 1, 2014, at 7:53 AM, Ben Pfaff <blp at nicira.com> wrote:
> > > 
> > >> On Mon, Apr 28, 2014 at 03:57:58PM -0700, Jarno Rajahalme wrote:
> > >> 
> > >>> On Mar 20, 2014, at 10:05 AM, Ben Pfaff <blp at nicira.com> wrote:
> > >>> 
> > >>>> On Fri, Mar 14, 2014 at 04:19:52PM +0900, Simon Horman wrote:
> > >>>> When creating a flow in the datapath as the result of an upcall
> > >>>> the match itself is the match supplied in the upcall while
> > >>>> the mask of the match, if supplied, is generated based on the
> > >>>> flow and mask composed during action translation.
> > >>>> 
> > >>>> In the case of, for example a UDP packet, the match will include
> > >>>> of L2, L3 and L4 fields. However, if the flow is cleared in
> > >>>> flow_push_mpls() then the mask that is synthesised from it will
> > >>>> not include L3 and L4 fields. This seems incorrect and the kernel
> > >>>> datapath complains about this mismatch.
> > >>>> 
> > >>>> Signed-off-by: Simon Horman <horms at verge.net.au>
> > >>> 
> > >>> The goal of clearing the fields is to ensure that later flow tables
> > >>> can't match on fields that aren't visible anymore.  That's important for
> > >>> accurate OpenFlow implementation, so I'd rather not change it.  On the
> > >>> other hand, I see the point you're making, but I don't immediately
> > >>> understand why it happens that way.  After all, I can change fields with
> > >>> OpenFlow actions and the datapath flows work out OK, why doesn't this
> > >>> work out OK too?  Do you understand the reason?
> > >> 
> > >> As the flow?s dl_type is changed to an MPLS type, later non-MPLS rules
> > >> will not match on the modified flow. AFAIK, you can match on L3/L4
> > >> fields only by also matching on the corresponding dl_type as a
> > >> prerequisite, no?
> > > 
> > > Yes, that's true.
> > > 
> > >> If this holds, I?d rather not clear the fields so we can properly do a
> > >> set IPv4 action followed by an MPLS push action. Currently the the
> > >> MPLS action clears the flow values at the translation time set in the
> > >> preceding action, so that at the commit time the values intended for
> > >> set IPv4 action are lost.
> > > 
> > > Are you sure?  compose_mpls_push_action() call commit_odp_actions() to
> > > avoid this very problem.
> > > 
> > 
> > I did not notice this, sorry about that.
> > 
> > > Assuming I'm right about that, at this point what I really want is an
> > > example of a situation that's broken in the current code and not broken
> > > with this patch applied, so that I can understand exactly what we're
> > > getting at here.
> > > 
> > 
> > It seems that a check on the nw_proto before committing set ports
> > actions is enough to avoid introducing new problems as part of my
> > masked set actions patch series.
> 
> OK.
> 
> Simon, I guess that you still see a problem that we should fix.  Can you
> provide me an example that I can work through for myself?  I want
> everything to work well.

Sure, will do.



More information about the dev mailing list