[ovs-dev] [PATCH 6/8] vswitchd: Implement Link Aggregation Control Protocol.
blp at nicira.com
Tue Feb 1 21:18:41 UTC 2011
On Mon, Jan 31, 2011 at 01:23:13PM -0800, Ethan Jackson wrote:
> This commit implements LACP, a protocol which allows directly
> connected switches to automatically negotiate which links may
> participate in bonds. This commit disables LACP by default. Once
> sufficiently tested, LACP will be enabled in "active" mode on
> bonded ports, and "passive" mode on all others.
> Bug #4213.
Thanks for writing this up! We've wanted LACP support for a long time.
compose_lacp_packet() should call ofpbuf_prealloc_tailroom(b,
ETH_HEADER_LEN + LACP_PDU_LEN) before the first ofpbuf_put_zeros() call.
Otherwise if the second call has to reallocate memory then 'eth' will be
invalid. I see that its current caller always allocates enough memory,
In compose_lacp_packet() can't struct assignments be used instead of
memcpy calls, for the lacp_info copies? Also in lacp_process_packet(),
parse_lacp_packet() looks like it expects flow_extract() to have set up
the l3 pointer already, but its comment doesn't mention that.
s/it's/its/ in "Attached to it's aggregator" in bridge.c.
I don't understand the new logic in bond_link_status_update(). Can you
explain it? (Would it be obvious if I understood LACP?)
bond_run() is starting to have significant duplicate code in the two
cases. Should we add a helper function?
In bridge_normal_ofhook_cb(), it's better to write "ntohs(flow->dl_type)
== ETH_TYPE_LACP" as "flow->dl_type == htons(ETH_TYPE_LACP)" to make it
easier for the compiler to optimize.
Does anything ensure that a flow will be reevaluated if the new factors
added to is_admissible() could cause its admissibility to change?
I see a couple of instances of 65335 (UINT16_MAX - 200) in vswitch.xml.
Are those typos for 65535 (UINT16_MAX)?
More information about the dev