Alex and I had an off-line discussion. He will post a simpler version
of the patch soon.
On Fri, Dec 12, 2014 at 1:53 AM, Alex Wang <al...@nicira.com> wrote:
> Sorry, forgot to mention that this patch targets branch-2.3,
>
> Haven't tried, but should also be applicable to master~
>
> On Fri, Dec 12, 2014 at 1:55 AM, Alex Wang <al...@nicira.com> wrote:
>>
>> In current recirculation implementation, the flow misses (with
>> 'recirc_id' set) are always looked up on the receiving bridge's
>> internal flow table. However, the bond port may actually reside
>> on another bridge which gets connected to the receiving bridge
>> via patch port. Since the recirculation rules are pushed to the
>> other bridge's internal table, the flow lookup on the receiving
>> bridge will match nothing but the drop rule, causing unexpected
>> packet drops.
>>
>> This commit fixes the above bug via changing the lookup logic
>> such that the internal flow table is checked only when flow
>> miss handling reaches the bridge (ofproto) which owns the bond
>> port.
>>
>> VMware-BZ: 1362178
>>
>> Reported-by: Ansis Atteka <aatt...@nicira.com>
>> Signed-off-by: Alex Wang <al...@nicira.com>
>> ---
>> ofproto/bond.c | 25 +++++++++++++++++++++++++
>> ofproto/bond.h | 1 +
>> ofproto/ofproto-dpif-xlate.c | 14 ++++++++++++--
>> ofproto/ofproto-dpif.c | 19 ++++++++++---------
>> 4 files changed, 48 insertions(+), 11 deletions(-)
>>
>> diff --git a/ofproto/bond.c b/ofproto/bond.c
>> index 44e9df1..93b28f8 100644
>> --- a/ofproto/bond.c
>> +++ b/ofproto/bond.c
>> @@ -946,6 +946,31 @@ bond_may_recirc(const struct bond *bond, uint32_t
>> *recirc_id,
>> }
>> }
>>
>> +/* Given the unique 'recirc_id', finds the corresponding 'bond' and
>> + * returns the 'bond->ofproto'. */
>> +struct ofproto_dpif *
>> +bond_get_ofproto_by_recirc_id(uint32_t recirc_id)
>> +{
>> + struct bond *bond;
>> +
>> + if (!recirc_id) {
>> + goto err;
>> + }
>> +
>> + ovs_rwlock_rdlock(&rwlock);
>> + HMAP_FOR_EACH (bond, hmap_node, all_bonds) {
>> + if (bond->recirc_id == recirc_id) {
>> + ovs_rwlock_unlock(&rwlock);
>> +
>> + return bond->ofproto;
>> + }
>> + }
>> + ovs_rwlock_unlock(&rwlock);
>> +
>> +err:
>> + return NULL;
>> +}
>> +
>> void
>> bond_update_post_recirc_rules(struct bond* bond, const bool force)
>> {
>> diff --git a/ofproto/bond.h b/ofproto/bond.h
>> index d7cb715..b03eadf 100644
>> --- a/ofproto/bond.h
>> +++ b/ofproto/bond.h
>> @@ -124,4 +124,5 @@ void bond_rebalance(struct bond *);
>> void bond_update_post_recirc_rules(struct bond *, const bool force);
>> bool bond_may_recirc(const struct bond *, uint32_t *recirc_id,
>> uint32_t *hash_bias);
>> +struct ofproto_dpif *bond_get_ofproto_by_recirc_id(uint32_t recirc_id);
>> #endif /* bond.h */
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 4f77ac5..de9fa70 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -1846,8 +1846,16 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> const struct xport *peer = xport->peer;
>> struct flow old_flow = ctx->xin->flow;
>> enum slow_path_reason special;
>> + uint8_t table_id = 0;
>>
>> ctx->xbridge = peer->xbridge;
>> + /* If 'recirc_id' is set and 'ofproto' is the same as the bond's
>> + * 'ofproto', starts the look up from internal table. */
>> + if (flow->recirc_id
>> + && (peer->xbridge->ofproto
>> + == bond_get_ofproto_by_recirc_id(flow->recirc_id))) {
>> + table_id = TBL_INTERNAL;
>> + }
>> flow->in_port.ofp_port = peer->ofp_port;
>> flow->metadata = htonll(0);
>> memset(&flow->tunnel, 0, sizeof flow->tunnel);
>> @@ -1859,14 +1867,16 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> ctx->xout->slow |= special;
>> } else if (may_receive(peer, ctx)) {
>> if (xport_stp_forward_state(peer)) {
>> - xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
>> true);
>> + xlate_table_action(ctx, flow->in_port.ofp_port, table_id,
>> + true, true);
>> } else {
>> /* Forwarding is disabled by STP. Let OFPP_NORMAL and
>> the
>> * learning action look at the packet, then drop it. */
>> struct flow old_base_flow = ctx->base_flow;
>> size_t old_size = ofpbuf_size(&ctx->xout->odp_actions);
>> mirror_mask_t old_mirrors = ctx->xout->mirrors;
>> - xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
>> true);
>> + xlate_table_action(ctx, flow->in_port.ofp_port, table_id,
>> + true, true);
>> ctx->xout->mirrors = old_mirrors;
>> ctx->base_flow = old_base_flow;
>> ofpbuf_set_size(&ctx->xout->odp_actions, old_size);
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 46595f8..bdc0f1c 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -3192,7 +3192,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto,
>> struct flow *flow,
>> {
>> enum rule_dpif_lookup_verdict verdict;
>> enum ofputil_port_config config = 0;
>> - uint8_t table_id;
>> + uint8_t table_id = 0;
>>
>> if (ofproto_dpif_get_enable_recirc(ofproto)) {
>> /* Always exactly match recirc_id since datapath supports
>> @@ -3201,17 +3201,18 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto,
>> struct flow *flow,
>> wc->masks.recirc_id = UINT32_MAX;
>> }
>>
>> - /* Start looking up from internal table for post recirculation
>> flows
>> - * or packets. We can also simply send all, including normal
>> flows
>> - * or packets to the internal table. They will not match any post
>> - * recirculation rules except the 'catch all' rule that resubmit
>> - * them to table 0.
>> + /* If 'ofproto' is the same as 'bond's ofproto, starts looking up
>> + * from internal table for post recirculation flows or packets.
>> + * We can also simply send all, including normal flows or packets
>> + * to the internal table. They will not match any post
>> recirculation
>> + * rules except the 'catch all' rule that resubmit them to table
>> 0.
>> *
>> * As an optimization, we send normal flows and packets to table
>> 0
>> * directly, saving one table lookup. */
>> - table_id = flow->recirc_id ? TBL_INTERNAL : 0;
>> - } else {
>> - table_id = 0;
>> + if (flow->recirc_id
>> + && ofproto == bond_get_ofproto_by_recirc_id(flow->recirc_id))
>> {
>> + table_id = TBL_INTERNAL;
>> + }
>> }
>>
>> verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true,
>> --
>> 1.7.9.5
>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
