[ovs-dev] [PATCH 6/8] vswitchd: Implement Link Aggregation Control Protocol.

Ben Pfaff 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,
but still...

In compose_lacp_packet() can't struct assignments be used instead of
memcpy calls, for the lacp_info copies?  Also in lacp_process_packet(),
lacp_update_ifaces().

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)?

Thanks,

Ben.




More information about the dev mailing list