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 <al...@nicira.com> 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 <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
