On Tue, Apr 01, 2014 at 12:21:03PM +0900, Simon Horman wrote: > On Thu, Mar 20, 2014 at 10:05:44AM -0700, Ben Pfaff 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? > > Hi Ben, > > I have taken a closer look at this and I think that I now understand the > problem. I would like to propose the following:
Hi Ben, I'd be most grateful if you could take another moment to look over this. > > > From: Simon Horman <ho...@verge.net.au> > > odp-util: Do not set port mask of non-IP packets > > In the case that an flow for an IP packet has an mpls_push action applied > the L3 and L4 portions of the flow will be cleared in flow_push_mpls(). > > Without this change commit_set_port_action() will set the tp_src and tp_dst > mask for the flow to all-ones because the base and flow port values no > longer match. Even though there will be no corresponding set action for the > ports; because the flow is no longer IP. > > In this case where nw_proto is not part of the match this manifests > in a problem because the userspace datapath rejects flows whose masks > have non-zero values for tp_src or dp_dst if the nw_proto mask is > not all-ones. > > This patch resolves this problem by having commit_set_port_action() return > without doing anything if flow->nw_proto is zero. The same logic is present > in commit_set_nw_action(). > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 956fef1..b40f9bc 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -3778,6 +3778,11 @@ static void > commit_set_port_action(const struct flow *flow, struct flow *base, > struct ofpbuf *odp_actions, struct flow_wildcards *wc) > { > + /* Check if 'flow' really has an L3 header. */ > + if (!flow->nw_proto) { > + return; > + } > + > if (!is_ip_any(base) || (!base->tp_src && !base->tp_dst)) { > return; > } > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev