I'm not against this idea; It seems correct to me, to keep the cache the same even if we need to revalidate, because we will throw it away if revalidation fails anyway.
We already had a discussion about using the xcache even when revalidation is required:- http://openvswitch.org/pipermail/dev/2014-May/040440.html I wonder if we need to extend that, so that rather than clearing the xcache in the revalidation case, we keep it (and make sure that it is not overwritten by the xlate_actions__). Regardless, I would feel more comfortable with this patch going in, as the cases of creating xcache and learning are separate, and not handled correctly in that function currently. On 4 June 2014 08:26, Alex Wang <[email protected]> wrote: > After discussing the issue with few people, > > I think we should keep the old_xcache during the full revalidation. > > And in xlate_learn_action(), if may_learn is false, we should check if the > learned rule is in the old_xcache, if it is, we should copy the XC_LEARN > entry > to the new_xcache list (but we do not refresh the learnt rule again, since > may_learn to false). > > Thoughts? > > Alex Wang, > > > On Tue, Jun 3, 2014 at 10:39 AM, Alex Wang <[email protected]> wrote: > >> I confirm this patch fix the bug, but have a high level concern, >> >> >> Assume another learnt flow with hard_timeout=10 and was never hit after >> install, now, if there is frequent change to the flow table and >> need_revalidate >> is constantly 'true', this learnt flow will never timeout... since we >> always >> redo the flow_mod during revalidation, >> >> Let's discuss further on this scenario, I'm thinking about solutions, >> >> Thanks, >> Alex Wang, >> >> >> >> >> On Tue, Jun 3, 2014 at 2:59 AM, Joe Stringer <[email protected]> >> wrote: >> >>> 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 <[email protected]> >>> Signed-off-by: Joe Stringer <[email protected]> >>> --- >>> ofproto/ofproto-dpif-xlate.c | 58 >>> ++++++++++++++++++++++-------------------- >>> 1 file changed, 30 insertions(+), 28 deletions(-) >>> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index 068d54f..e8f3dbf 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -265,7 +265,9 @@ struct xc_entry { >>> uint16_t vid; >>> } bond; >>> struct { >>> - struct rule_dpif *rule; >>> + struct ofproto_dpif *ofproto; >>> + struct ofputil_flow_mod *fm; >>> + struct ofpbuf *ofpacts; >>> } learn; >>> struct { >>> struct ofproto_dpif *ofproto; >>> @@ -2977,34 +2979,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); >>> - /* 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, NULL); >>> + entry->u.learn.ofproto = ctx->xbridge->ofproto; >>> + entry->u.learn.fm = xmalloc(sizeof *entry->u.learn.fm); >>> + entry->u.learn.ofpacts = ofpbuf_new(1024); >>> + 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); >>> } >>> } >>> >>> @@ -3911,12 +3917,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: >>> @@ -3989,8 +3991,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); >>> -- >>> 1.7.10.4 >>> >>> >> >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
