Good catch. Thanks for pointing it out. I will send out patch to fix this.
On Tue, Apr 29, 2014 at 2:14 AM, Simon Horman <ho...@verge.net.au> wrote:
> 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
