[ovs-dev] [PATCH 2/2] bridge: Refactor bridge_reconfigure().
blp at nicira.com
Mon Apr 23 13:12:23 PDT 2012
On Sun, Apr 22, 2012 at 02:22:48PM -0700, Ethan Jackson wrote:
> The existing bridge_reconfigure() implementation is sub-optimal.
> 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 book keeping is done,
That's just one word: "bookkeeping". (One of the few English words
with three consecutive sets of doubled letters.)
> and a list of future work is compiled. The bridge begins whittling
> down this list, and stops processing database changes until
> Bug #10902.
> Signed-off-by: Ethan Jackson <ethan at nicira.com>
Thank you for doing this work. Clearly it was a lot of work but I
think it's right approach. I only spotted one bug, but I have a lot
of comments anyway.
I think that the intention is for bridge_reconfigure() to run, then
bridge_reconfigure_continue() to run until we're done, without
bridge_reconfigure() ever being called again until we've finished. At
least, it looks like that's *almost* what we're assured will happen,
with the exception that if VLAN splinters change then we'll restart
reconfiguring in the middle. I think the latter is a bug. If I'm
right, I'd fix the bug and then add "assert(!reconfiguring);" early on
in bridge_reconfigure(), and perhaps "assert(reconfiguring);" early on
bridge_reconfigure_ofp() has the following comment:
/* Do any work needed to complete configuration. Don't consider ourselves
* done unless no work was needed in this iteration. This guarantees that
* ofproto_run() has a chance to respond to newly added ports. */
I don't understand the implications. What is the effect if
ofproto_run() does not get a chance to look at newly added ports?
Does the existing code do this?
Style, grammar, etc.
The names "ofpp_inmate" and "ofpp_death_row" seem mildly distasteful.
In bridge_queue_if_cfg(), using list_push_back() instead of
list_insert() would make the intent more obvious, without changing
In the function-level comment on bridge_add_del_ports(), s/it's/its/.
In bridge_refresh_ofp_port(), s/it's/its/ here:
/* Clear each "struct iface"s ofp_port so we can get it's correct value. */
The LIST_FOR_EACH_SAFE line in bridge_reconfigure_ofp() should be
indented one more space.
In iface_init(), the following two lines may be joined into one:
error = ofproto_port_add(br->ofproto, iface->netdev,
I think that it might make sense to merge bridge_update_ifaces() into
The functions iface_from_if_cfg(), iface_create(), and iface_init()
are closely related. iface_from_if_cfg() is the only caller of
iface_create(). iface_from_if_cfg() is always called just before
iface_init(), except that one caller makes an intervening call to
iface_set_ofp_port(). I think you should consider tying these
functions together more tightly. You could easily combine
iface_create() and iface_from_if_cfg(), and then you could combine
iface_init() also if you added an 'ofp_port' argument that the caller
could optionally pass in.
Cleanups (and possible performance improvements)
bridge_refresh_ofp_port() does redundant queries of the kernel
datapath if there are ports to be added: first it queries for each
if_cfg, then it iterates over every port. If the "if_cfg"s were
indexed on name (e.g. as an hmap indexed on ->cfg->name, instead of a
list), then during iteration it could easily add interfaces for any
datapath ports that have now appeared.
bridge_populate_ofpp_death_row() does a second iteration over the
kernel datapath port list. It would avoid doubling the kernel
communication on reconfiguration if we could combine this with the
similar iteration in bridge_refresh_ofp_port(). I don't see any real
barriers to doing that.
In bridge_update_ofprotos(), the loop that deletes ports named after
bridges will normally iterate over every port in every ofproto. Most
of the time that effort is entirely wasted. Could we add an "if"
condition around the whole loop that skips the whole thing if there
are no new ofprotos to be added? Alternatively, one could instead
move this logic into the final loop that creates ofprotos. That
for each bridge
for each port
delete the port if another bridge has that name
for each bridge
if there's no ofproto:
create the ofproto
change it to:
for each bridge A:
if there's no ofproto:
for each bridge B with B->ofproto != NULL:
if port_query_by_name(B->ofproto, A->name):
delete that port from B
create A's ofproto
which never iterates over every port and seems to me to be more
bridge_run() always commits two database transactions when we
reconfigure now. It'd be better to just commit one.
More information about the dev