[ovs-dev] [PACKET_OUT v2] ofproto-dpif: treat non-datapath ports as local port for OFPT_PACKET_OUT

[Simon Horman]

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 <horms at verge.net.au>
> Signed-off-by: Andy Zhou <azhou at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>