On Wed, Jan 20, 2016 at 11:31:07AM -0500, Russell Bryant wrote:
> Before this patch, physical.c build up the set of local datapaths for
> its own use. I'd like to use it in another module in a later patch, so
> pull it out of physical. It's now populated by the bindings module,
> since that seems like a more appropriate place to do it, and it's also
> done much earlier in the main loop, making it easier to re-use.
>
> Signed-off-by: Russell Bryant <russ...@ovn.org>
> Acked-by: Han Zhou <zhou...@gmail.com>
> ---
>
> v1->v2:
> - added ack from Han Zhou
I guess I reviewed v1 accidentally. I'll copy it here.
The scoping is weird. The hmap is torn down and recreated on each main
loop iteration, but instead of constructing it at the beginning of an
iteration and tearing it down at the end, it's torn down and then
rebuilt at the beginning, which means that after the main loop we have
to take another shot at tearing it down again.
The method of tearing down the hmap is needlessly inefficient. Instead
of iterating through the datapaths and removing them one by one, each
time we start over from the beginning and look for the first one. This
isn't new but I didn't notice it before:
> + /* Clear local_datapaths, as we're going to rebuild it next. */
> + struct hmap_node *node;
> + while ((node = hmap_first(local_datapaths))) {
> + hmap_remove(local_datapaths, node);
> + free(node);
> + }
I guess the obvious HMAP_FOR_EACH_SAFE isn't being used because there's
no outer struct. Maybe an outer struct should be defined, even if it's
just a bare wrapper around hmap_node.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
