The logic of the patch looks good to me. Thanks for fixing it.
The diff of the ofproto-dpif.h looks bigger than the actual change
(add an inline function). Do you know why?
Acked-by: Andy Zhou <az...@nicira.com>
On Fri, Dec 12, 2014 at 5:52 PM, 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 keeping lookup the misses
> (with 'recirc_id' set) in default table (table 0) and processing
> it until reaching the bridge that owns the bond port. Then,
> the misses can hit the post recirculation flows as expected.
>
> VMware-BZ: 1362178
>
> Reported-by: Ansis Atteka <aatt...@nicira.com>
> Signed-off-by: Alex Wang <al...@nicira.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 7 ++-
> ofproto/ofproto-dpif.c | 35 ++++-------
> ofproto/ofproto-dpif.h | 139
> ++++++++++++++++++++++--------------------
> 3 files changed, 91 insertions(+), 90 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 4f77ac5..0fc5443 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1846,6 +1846,7 @@ 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 =
> rule_dpif_lookup_get_init_table_id(&ctx->xin->flow);
>
> ctx->xbridge = peer->xbridge;
> flow->in_port.ofp_port = peer->ofp_port;
> @@ -1859,14 +1860,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..7204ccf 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1236,31 +1236,31 @@ add_internal_flows(struct ofproto_dpif *ofproto)
> return error;
> }
>
> - /* Continue non-recirculation rule lookups from table 0.
> + /* Drop any run away non-recirc rule lookups. Recirc_id has to be
> + * zero when reaching this rule.
> *
> - * (priority=2), recirc=0, actions=resubmit(, 0)
> + * (priority=2), recirc_id=0, actions=drop
> */
> - resubmit = ofpact_put_RESUBMIT(&ofpacts);
> - resubmit->ofpact.compat = 0;
> - resubmit->in_port = OFPP_IN_PORT;
> - resubmit->table_id = 0;
> -
> + ofpbuf_clear(&ofpacts);
> match_init_catchall(&match);
> match_set_recirc_id(&match, 0);
> -
> error = ofproto_dpif_add_internal_flow(ofproto, &match, 2, &ofpacts,
> &unused_rulep);
> if (error) {
> return error;
> }
>
> - /* Drop any run away recirc rule lookups. Recirc_id has to be
> - * non-zero when reaching this rule.
> + /* Continue rule lookups for not-matched recirc rules from table 0.
> *
> - * (priority=1), *, actions=drop
> + * (priority=1), actions=resubmit(, 0)
> */
> - ofpbuf_clear(&ofpacts);
> + resubmit = ofpact_put_RESUBMIT(&ofpacts);
> + resubmit->ofpact.compat = 0;
> + resubmit->in_port = OFPP_IN_PORT;
> + resubmit->table_id = 0;
> +
> match_init_catchall(&match);
> +
> error = ofproto_dpif_add_internal_flow(ofproto, &match, 1, &ofpacts,
> &unused_rulep);
>
> @@ -3200,16 +3200,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct
> flow *flow,
> if (wc) {
> 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.
> - *
> - * 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;
> + table_id = rule_dpif_lookup_get_init_table_id(flow);
> } else {
> table_id = 0;
> }
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index e49616e..58e4f22 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -87,72 +87,6 @@ extern struct ovs_rwlock xlate_rwlock;
> size_t ofproto_dpif_get_max_mpls_depth(const struct ofproto_dpif *);
> bool ofproto_dpif_get_enable_recirc(const struct ofproto_dpif *);
>
> -uint8_t rule_dpif_lookup(struct ofproto_dpif *, struct flow *,
> - struct flow_wildcards *, struct rule_dpif **rule,
> - bool take_ref);
> -
> -enum rule_dpif_lookup_verdict rule_dpif_lookup_from_table(struct
> ofproto_dpif *,
> - const struct flow
> *,
> - struct
> flow_wildcards *,
> - bool
> force_controller_on_miss,
> - uint8_t *table_id,
> - struct rule_dpif
> **rule,
> - bool take_ref);
> -
> -static inline void rule_dpif_ref(struct rule_dpif *);
> -static inline void rule_dpif_unref(struct rule_dpif *);
> -
> -void rule_dpif_credit_stats(struct rule_dpif *rule ,
> - const struct dpif_flow_stats *);
> -
> -static inline bool rule_dpif_is_fail_open(const struct rule_dpif *);
> -static inline bool rule_dpif_is_table_miss(const struct rule_dpif *);
> -static inline bool rule_dpif_is_internal(const struct rule_dpif *);
> -
> -uint8_t rule_dpif_get_table(const struct rule_dpif *);
> -
> -bool table_is_internal(uint8_t table_id);
> -
> -const struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *);
> -
> -ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule);
> -
> -void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
> - uint16_t hard_timeout);
> -
> -void choose_miss_rule(enum ofputil_port_config,
> - struct rule_dpif *miss_rule,
> - struct rule_dpif *no_packet_in_rule,
> - struct rule_dpif **rule, bool take_ref);
> -
> -bool group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
> - struct group_dpif **group);
> -
> -void group_dpif_release(struct group_dpif *group);
> -
> -void group_dpif_get_buckets(const struct group_dpif *group,
> - const struct list **buckets);
> -enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group);
> -
> -bool ofproto_has_vlan_splinters(const struct ofproto_dpif *);
> -ofp_port_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
> - ofp_port_t realdev_ofp_port,
> - ovs_be16 vlan_tci);
> -bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
> -
> -int ofproto_dpif_execute_actions(struct ofproto_dpif *, const struct flow *,
> - struct rule_dpif *, const struct ofpact *,
> - size_t ofpacts_len, struct ofpbuf *)
> - OVS_EXCLUDED(xlate_rwlock);
> -void ofproto_dpif_send_packet_in(struct ofproto_dpif *,
> - struct ofproto_packet_in *);
> -bool ofproto_dpif_wants_packet_in_on_miss(struct ofproto_dpif *);
> -int ofproto_dpif_send_packet(const struct ofport_dpif *, struct ofpbuf *);
> -void ofproto_dpif_flow_mod(struct ofproto_dpif *, struct ofputil_flow_mod *);
> -struct rule_dpif *ofproto_dpif_refresh_rule(struct rule_dpif *);
> -
> -struct ofport_dpif *odp_port_to_ofport(const struct dpif_backer *,
> odp_port_t);
> -
> /*
> * Recirculation
> * =============
> @@ -229,6 +163,79 @@ enum { N_TABLES = 255 };
> enum { TBL_INTERNAL = N_TABLES - 1 }; /* Used for internal hidden rules.
> */
> BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
>
> +uint8_t rule_dpif_lookup(struct ofproto_dpif *, struct flow *,
> + struct flow_wildcards *, struct rule_dpif **rule,
> + bool take_ref);
> +
> +/* If 'recirc_id' is set, starts looking up from internal table for
> + * post recirculation flows or packets. Otherwise, starts from table 0. */
> +static inline uint8_t
> +rule_dpif_lookup_get_init_table_id(const struct flow *flow)
> +{
> + return flow->recirc_id ? TBL_INTERNAL : 0;
> +}
> +
> +enum rule_dpif_lookup_verdict rule_dpif_lookup_from_table(struct
> ofproto_dpif *,
> + const struct flow
> *,
> + struct
> flow_wildcards *,
> + bool
> force_controller_on_miss,
> + uint8_t *table_id,
> + struct rule_dpif
> **rule,
> + bool take_ref);
> +
> +static inline void rule_dpif_ref(struct rule_dpif *);
> +static inline void rule_dpif_unref(struct rule_dpif *);
> +
> +void rule_dpif_credit_stats(struct rule_dpif *rule ,
> + const struct dpif_flow_stats *);
> +
> +static inline bool rule_dpif_is_fail_open(const struct rule_dpif *);
> +static inline bool rule_dpif_is_table_miss(const struct rule_dpif *);
> +static inline bool rule_dpif_is_internal(const struct rule_dpif *);
> +
> +uint8_t rule_dpif_get_table(const struct rule_dpif *);
> +
> +bool table_is_internal(uint8_t table_id);
> +
> +const struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *);
> +
> +ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule);
> +
> +void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
> + uint16_t hard_timeout);
> +
> +void choose_miss_rule(enum ofputil_port_config,
> + struct rule_dpif *miss_rule,
> + struct rule_dpif *no_packet_in_rule,
> + struct rule_dpif **rule, bool take_ref);
> +
> +bool group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
> + struct group_dpif **group);
> +
> +void group_dpif_release(struct group_dpif *group);
> +
> +void group_dpif_get_buckets(const struct group_dpif *group,
> + const struct list **buckets);
> +enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group);
> +
> +bool ofproto_has_vlan_splinters(const struct ofproto_dpif *);
> +ofp_port_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
> + ofp_port_t realdev_ofp_port,
> + ovs_be16 vlan_tci);
> +bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
> +
> +int ofproto_dpif_execute_actions(struct ofproto_dpif *, const struct flow *,
> + struct rule_dpif *, const struct ofpact *,
> + size_t ofpacts_len, struct ofpbuf *)
> + OVS_EXCLUDED(xlate_rwlock);
> +void ofproto_dpif_send_packet_in(struct ofproto_dpif *,
> + struct ofproto_packet_in *);
> +bool ofproto_dpif_wants_packet_in_on_miss(struct ofproto_dpif *);
> +int ofproto_dpif_send_packet(const struct ofport_dpif *, struct ofpbuf *);
> +void ofproto_dpif_flow_mod(struct ofproto_dpif *, struct ofputil_flow_mod *);
> +struct rule_dpif *ofproto_dpif_refresh_rule(struct rule_dpif *);
> +
> +struct ofport_dpif *odp_port_to_ofport(const struct dpif_backer *,
> odp_port_t);
>
> /* struct rule_dpif has struct rule as it's first member. */
> #define RULE_CAST(RULE) ((struct rule *)RULE)
> --
> 1.7.9.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
