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 461053a..d3912c1 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 507c565..6bac962 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 7174f2c..e2c7fb6 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);