diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c index 9224ee2e6..2e1fcb3a6 100644 --- a/ofproto/ofproto-dpif-xlate-cache.c +++ b/ofproto/ofproto-dpif-xlate-cache.c @@ -125,7 +125,7 @@ xlate_push_stats_entry(struct xc_entry *entry, case XC_LEARN: { enum ofperr error; error = ofproto_flow_mod_learn(entry->learn.ofm, true, - entry->learn.limit, NULL); + entry->learn.limit, NULL, stats->used); if (error) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL(&rl, "xcache LEARN action execution failed."); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 47ea0f47e..d608a5f25 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -5700,8 +5700,16 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn) if (!error) { bool success = true; if (ctx->xin->allow_side_effects) { + long long int last_used; + + if (ctx->xin->resubmit_stats) { + last_used = ctx->xin->resubmit_stats->used; + } else { + last_used = time_msec(); + } error = ofproto_flow_mod_learn(ofm, ctx->xin->xcache != NULL, - learn->limit, &success); + learn->limit, &success, + last_used); } else if (learn->limit) { if (!ofm->temp_rule || ofm->temp_rule->state != RULE_INSERTED) { diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index e22ca757a..ba5706f6a 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4880,7 +4880,7 @@ packet_xlate(struct ofproto *ofproto_, struct ofproto_packet_out *opo) if (entry->type == XC_LEARN) { struct ofproto_flow_mod *ofm = entry->learn.ofm; - error = ofproto_flow_mod_learn_refresh(ofm); + error = ofproto_flow_mod_learn_refresh(ofm, time_msec()); if (error) { goto error_out; } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 143ded690..9f7b8b6e8 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -2027,9 +2027,11 @@ enum ofperr ofproto_flow_mod_init_for_learn(struct ofproto *, struct ofproto_flow_mod *) OVS_EXCLUDED(ofproto_mutex); enum ofperr ofproto_flow_mod_learn(struct ofproto_flow_mod *, bool keep_ref, - unsigned limit, bool *below_limit) + unsigned limit, bool *below_limit, + long long int last_used) OVS_EXCLUDED(ofproto_mutex); -enum ofperr ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm); +enum ofperr ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm, + long long int last_used); enum ofperr ofproto_flow_mod_learn_start(struct ofproto_flow_mod *ofm) OVS_REQUIRES(ofproto_mutex); void ofproto_flow_mod_learn_revert(struct ofproto_flow_mod *ofm) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index dbf4958bc..e78c80d11 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -5472,7 +5472,8 @@ ofproto_flow_mod_init_for_learn(struct ofproto *ofproto, } enum ofperr -ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm) +ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm, + long long int last_used) { enum ofperr error = 0; @@ -5493,9 +5494,37 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm) * this function is executed the rule will be reinstated. */ if (rule->state == RULE_REMOVED) { struct cls_rule cr; + struct oftable *table = &rule->ofproto->tables[rule->table_id]; + ovs_version_t tables_version = rule->ofproto->tables_version; + + if (!cls_rule_visible_in_version(&rule->cr, tables_version)) { + const struct cls_rule *curr_cls_rule; + + /* Only check for matching classifier rules and their modified + * time, instead of also checking all rule metadata, with the goal + * of suppressing a learn action update that would replace a more + * recent rule in the classifier. */ + curr_cls_rule = classifier_find_rule_exactly(&table->cls, + &rule->cr, + tables_version); + if (curr_cls_rule) { + struct rule *curr_rule = rule_from_cls_rule(curr_cls_rule); + long long int curr_last_used; + + ovs_mutex_lock(&curr_rule->mutex); + curr_last_used = curr_rule->modified; + ovs_mutex_unlock(&curr_rule->mutex); + + if (curr_last_used > last_used) { + /* In the case of a newer visible rule, don't recreate the + * current rule. */ + return 0; + } + } + } - cls_rule_clone(&cr, &rule->cr); ovs_mutex_lock(&rule->mutex); + cls_rule_clone(&cr, &rule->cr); error = ofproto_rule_create(rule->ofproto, &cr, rule->table_id, rule->flow_cookie, rule->idle_timeout, @@ -5506,6 +5535,7 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm) rule->match_tlv_bitmap, rule->ofpacts_tlv_bitmap, &ofm->temp_rule); + ofm->temp_rule->modified = last_used; ovs_mutex_unlock(&rule->mutex); if (!error) { ofproto_rule_unref(rule); /* Release old reference. */ @@ -5513,7 +5543,7 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm) } else { /* Refresh the existing rule. */ ovs_mutex_lock(&rule->mutex); - rule->modified = time_msec(); + rule->modified = last_used; ovs_mutex_unlock(&rule->mutex); } return error; @@ -5565,10 +5595,16 @@ ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm, /* Refresh 'ofm->temp_rule', for which the caller holds a reference, if already * in the classifier, insert it otherwise. If the rule has already been - * removed from the classifier, a new rule is created using 'ofm->temp_rule' as - * a template and the reference to the old 'ofm->temp_rule' is freed. If - * 'keep_ref' is true, then a reference to the current rule is held, otherwise - * it is released and 'ofm->temp_rule' is set to NULL. + * removed from the classifier and replaced by another rule, the 'last_used' + * parameter is used to determine whether the newer rule is replaced or kept. + * If 'last_used' is greater than the last modified time of an identical rule + * in the classifier, then a new rule is created using 'ofm->temp_rule' as a + * template and the reference to the old 'ofm->temp_rule' is freed. If the + * rule has been removed but another identical rule doesn't exist in the + * classifier, then it will be recreated. If the rule hasn't been removed + * from the classifier, then 'last_used' is used to update the rules modified + * time. If 'keep_ref' is true, then a reference to the current rule is held, + * otherwise it is released and 'ofm->temp_rule' is set to NULL. * * If 'limit' != 0, insertion will fail if there are more than 'limit' rules * in the same table with the same cookie. If insertion succeeds, @@ -5579,10 +5615,11 @@ ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm, * during the call. */ enum ofperr ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref, - unsigned limit, bool *below_limitp) + unsigned limit, bool *below_limitp, + long long int last_used) OVS_EXCLUDED(ofproto_mutex) { - enum ofperr error = ofproto_flow_mod_learn_refresh(ofm); + enum ofperr error = ofproto_flow_mod_learn_refresh(ofm, last_used); struct rule *rule = ofm->temp_rule; bool below_limit = true; @@ -5615,6 +5652,11 @@ ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref, error = ofproto_flow_mod_learn_start(ofm); if (!error) { + /* ofproto_flow_mod_learn_start may have overwritten + * modified with current time. */ + ovs_mutex_lock(&ofm->temp_rule->mutex); + ofm->temp_rule->modified = last_used; + ovs_mutex_unlock(&ofm->temp_rule->mutex); error = ofproto_flow_mod_learn_finish(ofm, NULL); } } else { diff --git a/tests/learn.at b/tests/learn.at index d127fed34..d0bcc8363 100644 --- a/tests/learn.at +++ b/tests/learn.at @@ -836,3 +836,63 @@ AT_CHECK([ovs-vsctl add-br br1 -- set b br1 datapath_type=dummy]) OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([learning action - flapping learn rule]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 3 + +AT_CHECK([ovs-appctl time/stop], [0], [ignore]) +AT_CHECK([[ovs-ofctl add-flow br0 'table=0,priority=2,in_port=1,actions=resubmit(,2)']]) +AT_CHECK([[ovs-ofctl add-flow br0 'table=0,priority=2,in_port=2,actions=resubmit(,2)']]) +AT_CHECK([[ovs-ofctl add-flow br0 'table=2,actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:OXM_OF_IN_PORT[]),output:3']]) + +packet="eth(src=50:54:00:00:00:06,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)" + +dnl Run this test a few times in a loop to reduce the likelyhood that it passes by chance. +for i in 1 2 3; do + AT_CHECK([ovs-appctl revalidator/pause], [0]) + AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + + AT_CHECK([ovs-appctl revalidator/resume], [0]) + AT_CHECK([ovs-appctl revalidator/wait], [0]) + + AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | sort | grep 0x123], [0], [dnl + cookie=0x123, hard_timeout=3, priority=1,dl_dst=50:54:00:00:00:06 actions=output:1 + table=2, actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:3 +]) + + AT_CHECK([ovs-appctl revalidator/pause], [0]) + AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0]) + AT_CHECK([ovs-appctl time/warp 75], [0], [ignore]) + + AT_CHECK([ovs-appctl revalidator/resume], [0]) + AT_CHECK([ovs-appctl revalidator/wait], [0]) + + AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | sort | grep 0x123], [0], [dnl + cookie=0x123, hard_timeout=3, priority=1,dl_dst=50:54:00:00:00:06 actions=output:2 + table=2, actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:3 +]) +done + +dnl Wait and check for learned rule eviction due to hard timeout. +AT_CHECK([ovs-appctl time/warp 3200], [0], [ignore]) + +AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | grep 0x123], [0], [dnl + table=2, actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:3 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP