On Thu, Apr 24, 2014 at 10:34:15PM -0700, Andy Zhou wrote:
> 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 <az...@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) {
Clang tells me that if the above if statement if false...
> @@ -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) {
...then use_recirc is used unanitialised here.
This is because the if statement in question simply calls
goto done;
And it is clear from the context above that use_recirc is indeed
uninitialised in that case.
> + 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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
