It seems odd to me that we call it 'may_add_flows' in facet_account,
and 'do_learn_action' in action_xlate_ctx. I'd be inclined to call
both of them the same thing. It strikes me, that an appropriate name
for the flag may be "may_flow_mod" as that's the behavior we actually
care about. If we add actions in the future which flow_mod, this flag
would still be applicable.
Am I correct that this patch prevents leftover packets in the datapath
from updating the timeouts in learned actions in some cases? e.g. if
you change the actions of a rule, packets which applied to the old
rule may not be properly accounted? This seems like a fine trade-off,
just wanted to make sure I understand it.
Looks good, thanks.
Ethan
On Tue, Mar 20, 2012 at 15:44, Ben Pfaff <b...@nicira.com> wrote:
> "ovs-ofctl del-flows <bridge>" can result in the following call path:
>
> delete_flows_loose() in ofproto.c
> -> collect_rules_loose() -- uses 'ofproto_node' inside 'struct rule'
> -> rule_destruct() in ofproto-dpif.c
> -> facet_revalidate()
> -> facet_remove()
> -> facet_flush_stats()
> -> facet_account()
> -> xlate_actions()
> -> xlate_learn_action()
> -> ofproto_flow_mod() back in ofproto.c
> -> modify_flow_strict()
> -> collect_rules_strict() -- also uses 'ofproto_node'
>
> which goes "boom" when we fall back up the call chain because the nested
> use of ofproto_node steps on the outer use of ofproto_node.
>
> This commit fixes the problem by refusing to translate "learn" actions
> within facet_flush_stats(), breaking the doubled use.
>
> Another possible approach would be to switch to another way to keep track
> of rules in the flow_mod implementations, so that there'd be no fighting
> over 'ofproto_node'. But then "ovs-ofctl del-flows" might still leave some
> flows around (ones created by "learn" actions as flows are accounted as
> facets get deleted), which would be surprising behavior. And it seems in
> general a bad idea to allow recursive flow_mods; the consequences have not
> been carefully thought through.
>
> Before this commit, one can reproduce the problem by running an "hping3"
> between a couple of VMs plus the following commands on OVS in the middle.
> Sometimes you have to run them a few times:
>
> ovs-ofctl del-flows br0
> ovs-ofctl add-flow br0 "table=0 actions=learn(table=1, \
> idle_timeout=600, NXM_OF_VLAN_TCI[0..11], \
> NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], \
> output:NXM_OF_IN_PORT[], fin_idle_timeout=10), resubmit(,1)"
> ovs-ofctl add-flow br0 "table=1 priority=0 actions=flood"
>
> Bug #10184.
> Reported-by: Michael Mao <m...@nicira.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 34 +++++++++++++++++++++-------------
> 1 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 200846b..0ffc4e8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -209,11 +209,17 @@ struct action_xlate_ctx {
> * revalidating without a packet to refer to. */
> const struct ofpbuf *packet;
>
> - /* Should OFPP_NORMAL MAC learning and NXAST_LEARN actions execute? We
> - * want to execute them if we are actually processing a packet, or if we
> - * are accounting for packets that the datapath has processed, but not if
> - * we are just revalidating. */
> - bool may_learn;
> + /* Should OFPP_NORMAL update the MAC learning table? We want to update
> it
> + * if we are actually processing a packet, or if we are accounting for
> + * packets that the datapath has processed, but not if we are just
> + * revalidating. */
> + bool do_learn_macs;
> +
> + /* Should "learn" actions update the flow table? We want to update it if
> + * we are actually processing a packet, or in most cases if we are
> + * accounting for packets that the datapath has processed, but not if we
> + * are just revalidating. */
> + bool do_learn_action;
>
> /* The rule that we are currently translating, or NULL. */
> struct rule_dpif *rule;
> @@ -346,7 +352,7 @@ static void facet_flush_stats(struct facet *);
> static void facet_update_time(struct facet *, long long int used);
> static void facet_reset_counters(struct facet *);
> static void facet_push_stats(struct facet *);
> -static void facet_account(struct facet *);
> +static void facet_account(struct facet *, bool may_add_flows);
>
> static bool facet_is_controller_flow(struct facet *);
>
> @@ -2989,7 +2995,7 @@ update_stats(struct ofproto_dpif *p)
> facet->tcp_flags |= stats->tcp_flags;
>
> subfacet_update_time(subfacet, stats->used);
> - facet_account(facet);
> + facet_account(facet, true);
> facet_push_stats(facet);
> } else {
> if (!VLOG_DROP_WARN(&rl)) {
> @@ -3241,7 +3247,7 @@ facet_remove(struct facet *facet)
> }
>
> static void
> -facet_account(struct facet *facet)
> +facet_account(struct facet *facet, bool may_add_flows)
> {
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
> uint64_t n_bytes;
> @@ -3267,7 +3273,8 @@ facet_account(struct facet *facet)
> action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> facet->flow.vlan_tci,
> facet->rule, facet->tcp_flags, NULL);
> - ctx.may_learn = true;
> + ctx.do_learn_macs = true;
> + ctx.do_learn_action = may_add_flows;
> ofpbuf_delete(xlate_actions(&ctx, facet->rule->up.actions,
> facet->rule->up.n_actions));
> }
> @@ -3341,7 +3348,7 @@ facet_flush_stats(struct facet *facet)
> }
>
> facet_push_stats(facet);
> - facet_account(facet);
> + facet_account(facet, false);
>
> if (ofproto->netflow && !facet_is_controller_flow(facet)) {
> struct ofexpired expired;
> @@ -4964,7 +4971,7 @@ do_xlate_actions(const union ofp_action *in, size_t
> n_in,
>
> case OFPUTIL_NXAST_LEARN:
> ctx->has_learn = true;
> - if (ctx->may_learn) {
> + if (ctx->do_learn_action) {
> xlate_learn_action(ctx, (const struct nx_action_learn *) ia);
> }
> break;
> @@ -5017,7 +5024,8 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
> ctx->base_flow.vlan_tci = initial_tci;
> ctx->rule = rule;
> ctx->packet = packet;
> - ctx->may_learn = packet != NULL;
> + ctx->do_learn_macs = packet != NULL;
> + ctx->do_learn_action = packet != NULL;
> ctx->tcp_flags = tcp_flags;
> ctx->resubmit_hook = NULL;
> }
> @@ -5645,7 +5653,7 @@ xlate_normal(struct action_xlate_ctx *ctx)
> }
>
> /* Learn source MAC. */
> - if (ctx->may_learn) {
> + if (ctx->do_learn_macs) {
> update_learning_table(ctx->ofproto, &ctx->flow, vlan, in_bundle);
> }
>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
