[ovs-dev] [PATCH 2/2] ofproto-dpif: restore bond rebalance for non-recirc bond

[Andy Zhou]

Bond rebalancing was disabled for bonds not using recirculation. The
patch fixes this bug.

While fixing the bug, the bond_rebalance() was also restructured
slightly to move bond related logic back into ofproto/bond.c

Signed-off-by: Andy Zhou <azhou at nicira.com>
---
 ofproto/bond.c         | 21 +++++++++++++++------
 ofproto/bond.h         |  7 +++----
 ofproto/ofproto-dpif.c | 12 +++---------
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/ofproto/bond.c b/ofproto/bond.c
index 2fa65a9..f5a9d47 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -861,7 +861,7 @@ bond_entry_account(struct bond_entry *entry, uint64_t rule_tx_bytes)
 }
 
 /* Maintain bond stats using post recirculation rule byte counters.*/
-void
+static void
 bond_recirculation_account(struct bond *bond)
 {
     int i;
@@ -1087,15 +1087,15 @@ reinsert_bal(struct list *bals, struct bond_slave *slave)
  * The caller should have called bond_account() for each active flow, or in case
  * of recirculation is used, have called bond_recirculation_account(bond),
  * to ensure that flow data is consistently accounted at this point.
- *
- * Return whether rebalancing took place.*/
-bool
+ */
+void
 bond_rebalance(struct bond *bond)
 {
     struct bond_slave *slave;
     struct bond_entry *e;
     struct list bals;
     bool rebalanced = false;
+    bool use_recirc;
 
     ovs_rwlock_wrlock(&rwlock);
     if (!bond_is_balanced(bond) || time_msec() < bond->next_rebalance) {
@@ -1103,6 +1103,13 @@ bond_rebalance(struct bond *bond)
     }
     bond->next_rebalance = time_msec() + bond->rebalance_interval;
 
+    use_recirc = ofproto_dpif_get_enable_recirc(bond->ofproto) &&
+                 bond_may_recirc(bond, NULL, NULL);
+
+    if (use_recirc) {
+        bond_recirculation_account(bond);
+    }
+
     /* Add each bond_entry to its slave's 'entries' list.
      * Compute each slave's tx_bytes as the sum of its entries' tx_bytes. */
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
@@ -1158,7 +1165,7 @@ bond_rebalance(struct bond *bond)
             /* Re-sort 'bals'. */
             reinsert_bal(&bals, from);
             reinsert_bal(&bals, to);
-	    rebalanced = true;
+            rebalanced = true;
         } else {
             /* Can't usefully migrate anything away from 'from'.
              * Don't reconsider it. */
@@ -1174,8 +1181,10 @@ bond_rebalance(struct bond *bond)
     }
 
 done:
+    if (use_recirc && rebalanced) {
+        bond_update_post_recirc_rules(bond,true);
+    }
     ovs_rwlock_unlock(&rwlock);
-    return rebalanced;
 }
 
 /* Bonding unixctl user interface functions. */
diff --git a/ofproto/bond.h b/ofproto/bond.h
index e5ceb45..0d9a67a 100644
--- a/ofproto/bond.h
+++ b/ofproto/bond.h
@@ -96,7 +96,7 @@ void *bond_choose_output_slave(struct bond *, const struct flow *,
 /* Rebalancing. */
 void bond_account(struct bond *, const struct flow *, uint16_t vlan,
                   uint64_t n_bytes);
-bool bond_rebalance(struct bond *);
+void bond_rebalance(struct bond *);
 
 /* Recirculation
  *
@@ -104,9 +104,9 @@ bool bond_rebalance(struct bond *);
  *
  * When recirculation is used, each bond port is assigned with a unique
  * recirc_id. The output action to the bond port will be replaced by
- * a RECIRC action.
+ * a Hash action, followed by a RECIRC action.
  *
- *   ... actions= ... RECIRC(L4_HASH, recirc_id) ....
+ *   ... actions= ... HASH(hash(L4)), RECIRC(recirc_id) ....
  *
  * On handling first output packet, 256 post recirculation flows are installed:
  *
@@ -118,5 +118,4 @@ bool bond_rebalance(struct bond *);
 void bond_update_post_recirc_rules(struct bond *, const bool force);
 bool bond_may_recirc(const struct bond *, uint32_t *recirc_id,
                      uint32_t *hash_bias);
-void bond_recirculation_account(struct bond *);
 #endif /* bond.h */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 30cbb24..bf8757c 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1341,7 +1341,6 @@ run(struct ofproto *ofproto_)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
     uint64_t new_seq, new_dump_seq;
-    const bool enable_recirc = ofproto_dpif_get_enable_recirc(ofproto);
 
     if (mbridge_need_revalidate(ofproto->mbridge)) {
         ofproto->backer->need_revalidate = REV_RECONFIGURE;
@@ -1419,17 +1418,12 @@ run(struct ofproto *ofproto_)
 
         /* All outstanding data in existing flows has been accounted, so it's a
          * good time to do bond rebalancing. */
-        if (enable_recirc && ofproto->has_bonded_bundles) {
+        if (ofproto->has_bonded_bundles) {
             struct ofbundle *bundle;
 
             HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
-                struct bond *bond = bundle->bond;
-
-                if (bond && bond_may_recirc(bond, NULL, NULL)) {
-                    bond_recirculation_account(bond);
-                    if (bond_rebalance(bundle->bond)) {
-                        bond_update_post_recirc_rules(bond, true);
-                    }
+                if (bundle->bond) {
+                    bond_rebalance(bundle->bond);
                 }
             }
         }
-- 
1.9.1