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
