Thanks.
On Tue, Apr 29, 2014 at 02:30:26AM -0700, Andy Zhou wrote:
> 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
