On Thu, May 01, 2014 at 08:21:36AM -0700, Jarno Rajahalme wrote: > > > On May 1, 2014, at 7:53 AM, Ben Pfaff <b...@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 <b...@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 <ho...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
