From 0a93f63f64896b240fb3943122537f8b59f55f93 Mon Sep 17 00:00:00 2001 From: Joe Stringer Date: Tue, 3 Jun 2014 20:44:35 +1200 Subject: [PATCH] ofproto-dpif-xlate: Cache full flowmod for learning. Caching the results of xlate_learn was previously dependent on the state of the 'may_learn' flag. This meant that if the caller did not specify that this flow may learn, then a learn entry would not be cached. However, the xlate_cache tends to be used on a recurring basis, so failing to cache the learn entry can provide unexpected behaviour later on, particularly in corner cases. Such a corner case occurred previously:- * Revalidation was requested. * A flow with a learn action was dumped. * The flow had no packets. * The flow's corresponding xcache was cleared, and the flow revalidated. * The flow went on to receive packets after the xcache is re-created. In this case, the xcache would be re-created, but would not refresh the timeouts on the learnt flow until the next time it was cleared, even if it received more traffic. This would cause flows to time out sooner than expected. Symptoms of this bug may include unexpected forwarding behaviour or extraneous statistics being attributed to the wrong flow. This patch fixes the issue by caching the entire flow_mod, including actions, upon translating an xlate_learn action. This is used to perform a flow_mod from scratch with the original flow, rather than simply refreshing the rule that was created during the creation of the xcache. Bug #1252997. Reported-by: Scott Hendricks Signed-off-by: Joe Stringer Acked-by: Alex Wang Acked-by: Ben Pfaff --- ofproto/ofproto-dpif-xlate.c | 58 ++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index df4ec1775..64dfbfaed 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -267,7 +267,8 @@ struct xc_entry { } bond; struct { struct ofproto_dpif *ofproto; - struct rule_dpif *rule; + struct ofputil_flow_mod *fm; + struct ofpbuf *ofpacts; } learn; struct { struct ofproto_dpif *ofproto; @@ -2634,35 +2635,38 @@ xlate_bundle_action(struct xlate_ctx *ctx, } static void -xlate_learn_action(struct xlate_ctx *ctx, - const struct ofpact_learn *learn) +xlate_learn_action__(struct xlate_ctx *ctx, const struct ofpact_learn *learn, + struct ofputil_flow_mod *fm, struct ofpbuf *ofpacts) { - uint64_t ofpacts_stub[1024 / 8]; - struct ofputil_flow_mod fm; - struct ofpbuf ofpacts; + learn_execute(learn, &ctx->xin->flow, fm, ofpacts); + if (ctx->xin->may_learn) { + ofproto_dpif_flow_mod(ctx->xbridge->ofproto, fm); + } +} +static void +xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn) +{ ctx->xout->has_learn = true; - learn_mask(learn, &ctx->xout->wc); - if (!ctx->xin->may_learn) { - return; - } - - ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub); - learn_execute(learn, &ctx->xin->flow, &fm, &ofpacts); - ofproto_dpif_flow_mod(ctx->xbridge->ofproto, &fm); - ofpbuf_uninit(&ofpacts); - if (ctx->xin->xcache) { struct xc_entry *entry; entry = xlate_cache_add_entry(ctx->xin->xcache, XC_LEARN); - entry->u.learn.ofproto = ctx->xin->ofproto; - /* Lookup the learned rule, taking a reference on it. The reference - * is released when this cache entry is deleted. */ - rule_dpif_lookup(ctx->xbridge->ofproto, &ctx->xin->flow, NULL, - &entry->u.learn.rule, true); + entry->u.learn.ofproto = ctx->xbridge->ofproto; + entry->u.learn.fm = xmalloc(sizeof *entry->u.learn.fm); + entry->u.learn.ofpacts = ofpbuf_new(64); + xlate_learn_action__(ctx, learn, entry->u.learn.fm, + entry->u.learn.ofpacts); + } else if (ctx->xin->may_learn) { + uint64_t ofpacts_stub[1024 / 8]; + struct ofputil_flow_mod fm; + struct ofpbuf ofpacts; + + ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub); + xlate_learn_action__(ctx, learn, &fm, &ofpacts); + ofpbuf_uninit(&ofpacts); } } @@ -3579,12 +3583,8 @@ xlate_push_stats(struct xlate_cache *xcache, bool may_learn, break; case XC_LEARN: if (may_learn) { - struct rule_dpif *rule = entry->u.learn.rule; - - /* Reset the modified time for a rule that is equivalent to - * the currently cached rule. If the rule is not the exact - * rule we have cached, update the reference that we have. */ - entry->u.learn.rule = ofproto_dpif_refresh_rule(rule); + ofproto_dpif_flow_mod(entry->u.learn.ofproto, + entry->u.learn.fm); } break; case XC_NORMAL: @@ -3653,8 +3653,8 @@ xlate_cache_clear(struct xlate_cache *xcache) mbridge_unref(entry->u.mirror.mbridge); break; case XC_LEARN: - /* 'u.learn.rule' is the learned rule. */ - rule_dpif_unref(entry->u.learn.rule); + free(entry->u.learn.fm); + ofpbuf_delete(entry->u.learn.ofpacts); break; case XC_NORMAL: free(entry->u.normal.flow); -- 2.20.1