[ovs-dev] [non-NORMAL learning 3/3] vswitchd: Only re-learn from flows that output to OFPP_NORMAL.

Ben Pfaff blp at nicira.com
Wed Aug 4 17:27:37 UTC 2010


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.
---
 lib/tag.c         |    9 +++++++++
 lib/tag.h         |    4 +++-
 ofproto/ofproto.c |    2 +-
 ofproto/ofproto.h |    6 +++---
 vswitchd/bridge.c |   28 ++++++++++++++++++++--------
 5 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/lib/tag.c b/lib/tag.c
index 68f756a..1690ea1 100644
--- a/lib/tag.c
+++ b/lib/tag.c
@@ -54,6 +54,15 @@ tag_create_deterministic(uint32_t seed)
     return (1u << x) | (1u << y);
 }
 
+/* Returns a tag_type with a single arbitrary 1-bit.  This isn't a tag, since a
+ * tag has at least 2 distinct bits set.  Therefore it is nonzero, but it will
+ * not match any tag, which might come in handy to some caller. */
+tag_type
+tag_arbitrary_1bit(void)
+{
+    return 1;                   /* Arbitrary enough. */
+}
+
 /* Initializes 'set' as an empty tag set. */
 void
 tag_set_init(struct tag_set *set)
diff --git a/lib/tag.h b/lib/tag.h
index f645dc9..ef8f4ef 100644
--- a/lib/tag.h
+++ b/lib/tag.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008 Nicira Networks.
+ * Copyright (c) 2008, 2010 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -74,6 +74,8 @@ tag_type tag_create_deterministic(uint32_t seed);
 static inline bool tag_intersects(tag_type, tag_type);
 static inline bool tag_is_valid(tag_type);
 
+tag_type tag_arbitrary_1bit(void);
+
 /* Returns true if 'a' and 'b' have at least one tag in common,
  * false if their set of tags is disjoint. . */
 static inline bool
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 461053a..d3912c1 100644
--- 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
--- 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 12bad0b..06d81f1 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2470,13 +2470,21 @@ bridge_normal_ofhook_cb(const flow_t *flow, const struct ofpbuf *packet,
                         uint16_t *nf_output_iface, void *br_)
 {
     struct bridge *br = br_;
+    bool retval;
 
     COVERAGE_INC(bridge_process_flow);
-    return process_flow(br, flow, packet, actions, tags, nf_output_iface);
+
+    retval = process_flow(br, flow, packet, actions, tags, nf_output_iface);
+    if (*tags == 0) {
+        /* Ensure that '*tags' is nonzero, so bridge_account_flow_ofhook_cb()
+         * knows that it should use this flow to update the learning table. */
+        *tags = tag_arbitrary_1bit();
+    }
+    return retval;
 }
 
 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_)
@@ -2484,20 +2492,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);
-- 
1.7.1





More information about the dev mailing list