On Wed, Apr 30, 2014 at 03:01:20AM -0700, Andy Zhou wrote: > When controller sends OFPT_PACKET_OUT message with the in_port set > to a patch port or as CONTROLLER, and the message execution requires > recirculation, those packets will be dropped in the datapath. > This is because the post recirculation flow will not be set up by > Xlate layer that rejects up call without a valid datapath input port. > > This patch implements a reasonable solution by injecting packets' > in_port does not have a valid datapath port using LOCAL port's > datapath port id. > > Reported-by: Simon Horman <ho...@verge.net.au> > Signed-off-by: Andy Zhou <az...@nicira.com>
Thanks! I have tested this and it appears to work :) In particular it works with recirculation for both Bonding and my series to add support for recirculation for MPLS. In both cases this was without the patches I posted in the latest recirculation for MPLS series which execute recirculation in ovs-vswtichd for packet_out messages. Or in other words, its a much simpler solution than the one I proposed :) One side effect of this approach, which I'm not sure how I feel about, is that (as the patch clearly implies) the in_port for execution resulting from packet_out messages is changed to LOCAL. The implication for this is that anything that is relying on the in_port that was actually supplied in the packet out_message will not work. For instance a goto_table action that result in a match on should the in_port when looking up a rule in the new table. Something like this (I have not tested either scenario): I think this will fail to match but that may not be obvious to users: packet_out: in_port=CONTROLLER actions=goto_table:1 table 1: match=in_port=CONTROLLER actions=normal I think this will match but that may not be obvious to users: packet_out: in_port=CONTROLLER actions=goto_table:1 table 1: match=in_port=LOCAL actions=normal Where CONTROLLER could be any port covered by this patch. > -- > v1 -> v2: Also handles CONTROLLER as input port > --- > ofproto/ofproto-dpif.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 4cebd77..d1b1405 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -193,6 +193,7 @@ struct vlan_splinter { > static void vsp_remove(struct ofport_dpif *); > static void vsp_add(struct ofport_dpif *, ofp_port_t realdev_ofp_port, int > vid); > > +static odp_port_t packet_out_odp_port(const struct ofproto_dpif *, > ofp_port_t); > static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *, > ofp_port_t); > > @@ -3117,7 +3118,6 @@ ofproto_dpif_execute_actions(struct ofproto_dpif > *ofproto, > struct dpif_flow_stats stats; > struct xlate_out xout; > struct xlate_in xin; > - ofp_port_t in_port; > struct dpif_execute execute; > int error; > > @@ -3135,17 +3135,14 @@ ofproto_dpif_execute_actions(struct ofproto_dpif > *ofproto, > xin.resubmit_stats = &stats; > xlate_actions(&xin, &xout); > > - in_port = flow->in_port.ofp_port; > - if (in_port == OFPP_NONE) { > - in_port = OFPP_LOCAL; > - } > execute.actions = ofpbuf_data(&xout.odp_actions); > execute.actions_len = ofpbuf_size(&xout.odp_actions); > execute.packet = packet; > execute.md.tunnel = flow->tunnel; > execute.md.skb_priority = flow->skb_priority; > execute.md.pkt_mark = flow->pkt_mark; > - execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port); > + execute.md.in_port.odp_port = packet_out_odp_port(ofproto, > + > flow->in_port.ofp_port); > execute.needs_help = (xout.slow & SLOW_ACTION) != 0; > > error = dpif_execute(ofproto->backer->dpif, &execute); > @@ -4745,6 +4742,29 @@ ofp_port_to_odp_port(const struct ofproto_dpif > *ofproto, ofp_port_t ofp_port) > return ofport ? ofport->odp_port : ODPP_NONE; > } > > +/* Converts ofp port to odp port, similar to ofp_port_to_odp_port. > + * This function treats patch ports and CONTROLLER port as LOCAL port. > + * > + * In case the packet out execution requires recirculation, the post > + * recirculation upcall with a valid datapath in_port will not be rejected > + * by the xlate layer. */ > +static odp_port_t > +packet_out_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port) > +{ > + const struct ofport_dpif *ofport; > + > + if ((ofp_port == OFPP_NONE) || (ofp_port == OFPP_CONTROLLER)) { > + ofp_port = OFPP_LOCAL; > + } > + > + ofport = get_ofp_port(ofproto, ofp_port); > + if (ofport && netdev_vport_is_patch(ofport->up.netdev)) { > + ofport = get_ofp_port(ofproto, OFPP_LOCAL); > + } > + > + return ofport ? ofport->odp_port : ODPP_NONE; > +} > + > struct ofport_dpif * > odp_port_to_ofport(const struct dpif_backer *backer, odp_port_t odp_port) > { > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev