[ovs-dev] [lacp 07/10] vswitchd: Modularize LACP.
ethan at nicira.com
Thu Mar 17 14:58:39 PDT 2011
> The lacp_active member in struct port seems like a bit of a oddity.
> It seems to me that it could be fully integrated into the lacp object,
> and that that would be more natural anyhow.
This is actually fairly difficult to get rid of. In order to remove
it we would need to move the lacp_configure() function call into
port_reconfigure() so that we don't need store the lacp_active
configuration parameter until port_update_lacp(). Unfortunately, we
can't really move lacp_configure into port_reconfigure because it
requires port->bridge->ea which is not initialized yet. This is
certainly not an insurmountable problem. However lacp will shortly be
moving out of the bridge and the lacp_active parameter will go with.
I'm inclined to leave it until then.
> The old implementation of lacp_update_ifaces() called
> ofproto_revalidate(), but the new equivalent lacp_update_attached()
> function, understandably, does not. Was the revalidation not really
It wasn't really needed. Flows need to be revalidated when a slave's
enabled status changes. Lacp attached status may cause a given
slave's enabled status to change in which case it's flows will be
revalidated. But there doesn't exist a case where the attached status
changes, the enabled status doesn't change, and flows need to be
revalidated. This is because attached status really has nothing to do
with flows directly.
> Would you mind making send_pdu a parameter of lacp_run(), instead of a
> member in struct lacp? I see that lacp_run() is currently the only
> place that the callback is called, but I am always nervous about
> callbacks getting fired off in contexts where the caller does not
> expect it. Passing the callback only to the function that can
> actually call it makes it really clear that that is the only context
> that has to worry about the callback getting invoked.
This is a good idea. Cleaner.
An incremental is on it's way.
More information about the dev