I upgraded my clang from 3.4 to 3.5. Now I can also reproduce the
warning message.
On Tue, Apr 29, 2014 at 2:32 AM, Simon Horman <ho...@verge.net.au> wrote:
> 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
