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 <joestrin...@nicira.com> 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 <shendri...@vmware.com>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> 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
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
