vswitchd: Only re-learn from flows that output to OFPP_NORMAL.

Commit e96a4d8035 "bridge: Feed flow stats into learning table." started
feeding flow statistics back into the learning table, but it did not
distinguish between flows with and flows without an action that outputs to
OFPP_NORMAL.  Flows without such an action are not put into the learning
table initially, because bridge_normal_ofhook_cb() is not called for them,
but since that commit they have been put into the learning table when their
flows are reassessed.

This is inconsistent--flows without OFPP_NORMAL should either be learned
from all the time or never, not sometimes.  I can see valid arguments both
ways, but since it was always my intention not to learn from such flows,
this commit disables learning from them.

Problem found by code inspection.  I don't know of any observed bug that
this fixes.

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 461053ab430e80d5d381797fd1235f1b9719f76b..d3912c17b638483db56740f5677e9da3852a9542 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2062,7 +2062,7 @@ rule_account(struct ofproto *ofproto, struct rule *rule, uint64_t extra_bytes)
         && total_bytes > rule->accounted_bytes)
     {
         ofproto->ofhooks->account_flow_cb(
-            &rule->cr.flow, rule->odp_actions, rule->n_odp_actions,
+            &rule->cr.flow, rule->tags, rule->odp_actions, rule->n_odp_actions,
             total_bytes - rule->accounted_bytes, ofproto->aux);
         rule->accounted_bytes = total_bytes;
     }

diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 507c5656510d29ce9962f19c7f585b7d61420ac1..6bac96268c73be59ef15141bb519e3569fcd82fd 100644 (file)
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -145,9 +145,9 @@ struct ofhooks {
     bool (*normal_cb)(const flow_t *, const struct ofpbuf *packet,
                       struct odp_actions *, tag_type *,
                       uint16_t *nf_output_iface, void *aux);
-    void (*account_flow_cb)(const flow_t *, const union odp_action *,
-                            size_t n_actions, unsigned long long int n_bytes,
-                            void *aux);
+    void (*account_flow_cb)(const flow_t *, tag_type tags,
+                            const union odp_action *, size_t n_actions,
+                            unsigned long long int n_bytes, void *aux);
     void (*account_checkpoint_cb)(void *aux);
 };
 void ofproto_revalidate(struct ofproto *, tag_type);

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 7174f2c8e9b827b126bb44e8f1afe3306c86e726..e2c7fb68e0b00e116b6574327cdb8fd5e50c024b 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2470,11 +2470,12 @@ bridge_normal_ofhook_cb(const flow_t *flow, const struct ofpbuf *packet,
     struct bridge *br = br_;
 
     COVERAGE_INC(bridge_process_flow);
+
     return process_flow(br, flow, packet, actions, tags, nf_output_iface);
 }
 
 static void
-bridge_account_flow_ofhook_cb(const flow_t *flow,
+bridge_account_flow_ofhook_cb(const flow_t *flow, tag_type tags,
                               const union odp_action *actions,
                               size_t n_actions, unsigned long long int n_bytes,
                               void *br_)
@@ -2482,20 +2483,24 @@ bridge_account_flow_ofhook_cb(const flow_t *flow,
     struct bridge *br = br_;
     const union odp_action *a;
     struct port *in_port;
-    tag_type tags = 0;
+    tag_type dummy = 0;
     int vlan;
 
-    /* Feed information from the active flows back into the learning table
-     * to ensure that table is always in sync with what is actually flowing
-     * through the datapath. */
-    if (is_admissible(br, flow, false, &tags, &vlan, &in_port)) {
+    /* Feed information from the active flows back into the learning table to
+     * ensure that table is always in sync with what is actually flowing
+     * through the datapath.
+     *
+     * We test that 'tags' is nonzero to ensure that only flows that include an
+     * OFPP_NORMAL action are used for learning.  This works because
+     * bridge_normal_ofhook_cb() always sets a nonzero tag value. */
+    if (tags && is_admissible(br, flow, false, &dummy, &vlan, &in_port)) {
         update_learning_table(br, flow, vlan, in_port);
     }
 
+    /* Account for bond slave utilization. */
     if (!br->has_bonded_ports) {
         return;
     }
-
     for (a = actions; a < &actions[n_actions]; a++) {
         if (a->type == ODPAT_OUTPUT) {
             struct port *out_port = port_from_dp_ifidx(br, a->output.port);