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
