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 > finished. > > Bug #10902. > Signed-off-by: Ethan Jackson <[email protected]> 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. Bugs ---- 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 in bridge_reconfigure_continue(). Questions --------- 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 semantics. 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, &ofp_port); Cleanups -------- I think that it might make sense to merge bridge_update_ifaces() into bridge_add_del_ports(). 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 instead, of: 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 straightforward anyway. bridge_run() always commits two database transactions when we reconfigure now. It'd be better to just commit one. Thanks, Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
