On Sun, Apr 22, 2012 at 11:45:12PM -0700, Ethan Jackson wrote:
> The existing bridge_reconfigure() implementation is suboptimal.
> When adding lots of new ports, on every pass through the run loop
> it allocates a bunch of "struct iface"s and "struct port"s, only to
> destroy them when out of time. Additionally, when there are errors
> adding or deleting ports, it can fail to converge. Instead it will
> attempt and fail to add the same set of ports forever.
>
> This patch rewrites bridge_reconfigure() using a new strategy.
> Whenever the database changes, some initial bookkeeping is done,
> and a list of future work is compiled. The bridge begins whittling
> down this list, and stops processing database changes until
> finished.
>
> Bug #10902.
> Signed-off-by: Ethan Jackson <[email protected]>
This looks more or less good-to-go to me. Thank you!
(How much have you tested it? What kinds of tests did you do?)
Bug (?)
-------
In bridge_run(), this:
if (!reconf_txn) {
reconf_txn = ovsdb_idl_txn_create(idl);
}
should be moved inside the "if (cfg)" block just below it to be
consistent with behavior in the "if (!reconfiguring)" block above it,
I think.
Possible style issues/cleanups
------------------------------
The ofpp_garbage list could just be a dynamic array of "uint16_t"s,
although the current representation is OK too.
iface_create() has two function-level comments above it now. I think
the first one is stale:
/* Initialize 'iface' with a netdev and ofp_port if necessary. */
/* Creates a new iface on 'br' based on 'if_cfg'. The new iface has OpenFlow
* port number 'ofp_port'. If ofp_port is negative, an OpenFlow port is
* automatically allocated for the iface. */
Each call to iface_create() now has the same two lines of code after
it:
hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node);
free(if_cfg);
Should we integrate them into iface_create()?
iface_create() now contains:
if (ofp_port >= 0) {
iface_set_ofp_port(iface, ofp_port);
}
...
if (iface->netdev && iface->ofp_port < 0) {
uint16_t ofp_port;
int error;
error = ofproto_port_add(br->ofproto, iface->netdev, &ofp_port);
if (!error) {
VLOG_INFO("bridge %s: added interface %s (%d)", br->name,
iface->name, ofp_port);
iface_set_ofp_port(iface, ofp_port);
} else {
netdev_close(iface->netdev);
iface->netdev = NULL;
}
}
A possible alternative is:
...
if (iface->netdev && ofp_port < 0) {
uint16_t new_ofp_port;
int error;
error = ofproto_port_add(br->ofproto, iface->netdev, &new_ofp_port);
if (!error) {
ofp_port = new_ofp_port;
} else {
netdev_close(iface->netdev);
iface->netdev = NULL;
}
}
if (ofp_port >= 0) {
VLOG_INFO("bridge %s: added interface %s (%d)", br->name,
iface->name, ofp_port);
iface_set_ofp_port(iface, ofp_port);
}
It would work out more cleanly if we changed the "no port yet"
ofp_port parameter value to be OFPP_NONE, or if we changed the
semantics of the ofproto_port_add() output parameter to use -1. If
you don't make this change, though, please do notice that both the
function parameter and the inner block variable are named 'ofp_port'
currently.
There's a doubled blank line in bridge_add_del_ports().
There's a comment in bridge_run() that runs past column 79.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
