On Wed, Apr 30, 2014 at 3:53 AM, Simon Horman <ho...@verge.net.au> wrote:
> 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.
Thanks for testing this.
>
> 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.
>
Only ODP ports are changed to LOCAL, so simple rule matches as
outlined should work.
However, this scenario is valid when recirculation is involved. I am
not sure what we should
do about this edge case either.
>> --
>> 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

Reply via email to