[ovs-dev] [stable 3/3] bond: New bonding mode "stable".

Ben Pfaff blp at nicira.com
Thu Apr 14 17:16:09 UTC 2011


On Wed, Apr 13, 2011 at 05:12:05PM -0700, Ethan Jackson wrote:
> Stable bonds attempt to assign a given flow to the same slave
> consistently.

Could we use BM_STABLE as the enum?  It's easier to understand.  If
you want to keep the "stb" prefix or infix elsewhere that's fine with
me.

Could you add a comment on the new stb_idx member of struct
bond_slave?

In bond_reconfigure(), the guess of 2*n+1 for initial len_stb_slaves
seems kind of big.  Currently, anyhow, most bonds never get expanded
after they are initially set up.  I think I'd either use n or just 0
and let bond_stb_enable_slave() expand it.

In bond_stb_enable_slave(), the canonical way to implement an
expandable array in the OVS tree is to use x2nrealloc().  Could you
use that instead of open-coding it?

In bond_stb_sort_cmp__(), using "a - b" as the return value fails if
integer overflow can occur.  I don't think that that is possible here,
since LACP port ids are only 16 bits and the subtraction is done in 32
bits, and ifindex are always positive.  Nevertheless, I've seen enough
subtle bugs in this area that "a < b ? -1 : a > b" would make me more
comfortable.

You can avoid any casts in bond_stb_sort_cmp__() by adding "const" in
the right places, e.g. here's how compare_coverage_counters() does it:

    static int
    compare_coverage_counters(const void *a_, const void *b_)
    {
        const struct coverage_counter *const *ap = a_;
        const struct coverage_counter *const *bp = b_;
        const struct coverage_counter *a = *ap;
        const struct coverage_counter *b = *bp;
        if (a->count != b->count) {
            return a->count < b->count ? 1 : -1;
        } else {
            return strcmp(a->name, b->name);
        }
    }

In vswitch.xml, deleting the introductory paragraph makes
"balance-tcp" and "stable" subordinate to the paragraph that says
"Some kinds of bonding will work with any kind of upstream switch".  I
guess that's literally true, since these modes fall back to SLB if
LACP fails, but it's not what people really want.  I think I'd just
generalize the introductory paragraph, since it applies to both
"balance-tcp" and "stable".

In the description of "stable" in vswitch.xml, I think that there's
more to explain.  First, the stability holds across ovs-vswitchd runs,
because it's based on identifiers from the database (LACP IDs).  And
it's not just stability that this mode brings to the table, right?
It's additionally symmetry.  If it was only stability that was
important then we could just disable rebalancing and be done with it.

Thanks,

Ben.



More information about the dev mailing list