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
