ofproto-dpif-xlate: Don't reinstall removed XC_LEARN rule.

When the a revalidator thread is updating statistics for an XC_LEARN
xcache entry in xlate_push_stats_entry it uses ofproto_flow_mod_learn.
The revalidator will update stats for rules even if they are in a
removed state or marked as invisible. However, ofproto_flow_mod_learn
will detect if a flow has been removed and re-add it in that case. This
can result in an old learn action replacing the new learn action that
had replaced it in the first place.

This change adds a new last_used parameter to ofproto_flow_mod_learn
allowing the caller to provide a timestamp that will be fed into the
learned rule's modified time. The provided timestamp should be the time
of the last packet activity. If last_used is not set then the current
time is used, as is the current behaviour.

This change also adds a check when replacing a learned rule to favour the
newest rule.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2213892
Signed-off-by: Mike Pattrick <mkp@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

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.");

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) {

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;
                 }

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)

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
+ * removed from the classifier and replaced by another rule, the 'last_used'
- * a template and the reference to the old 'ofm->temp_rule' is freed.  If
+ * parameter is used to determine whether the newer rule is replaced or kept.
- * 'keep_ref' is true, then a reference to the current rule is held, otherwise
+ * If 'last_used' is greater than the last modified time of an identical rule
- * it is released and 'ofm->temp_rule' is set to NULL.
+ * 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 {

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