On Thu, Oct 18, 2012 at 12:51:53PM -0700, Justin Pettit wrote:
> In a future commit, we will make multiple bridges share a single backing
> datapath. Our simple mapping from datapath to OpenFlow port numbers
> won't work, since we'll want the same OpenFlow port numbers on different
> bridges. For example, the OFPP_LOCAL port must be the same on all
> bridges, but will have to be a different datapath port on the converged
> datapath.
>
> This commit makes it the responsibility of ofproto to assign the
> OpenFlow port numbers instead of doing a simple translation from the
> datapath ones.
>
> Signed-off-by: Justin Pettit <jpet...@nicira.com>
"git am" says:
Applying: Separate OpenFlow port numbers from datapath ones.
/home/blp/ovs/.git/rebase-apply/patch:230: trailing whitespace.
warning: 1 line adds whitespace errors.
Press any key to continue...
In the declaration of struct ofproto, 'ofp_requests' doesn't say what
each shash element points to, so the reader has to hunt for it. I
think that it points to a uint16_t; if so, then I think you might as
well use the specialized string to integer map type in simap.[ch],
which should also be more memory efficient.
In the declaration of struct ofport_dpif, I'd consider naming
'hmap_node' something more explicit, like 'odp_port_node'. Otherwise
I think there's a risk of confusion later with struct ofport's own
'hmap_node'.
We want to avoid quickly reusing OpenFlow port numbers. Previously,
that was indirectly implemented through dpif-linux, which makes an
effort to avoid quickly reusing kernel datapath port numbers. This
commit defeats that, I think, by generally allocating the
lowest-numbered available OpenFlow port number. Presumably we should
remove the reuse-avoidance logic from dpif-linux and move it to
ofproto?
In ofproto-dpif.c, in port_construct(), I'd consider checking that
odp_port_to_ofp_port() returns OFPP_NONE for the new ODP port number.
Otherwise it seems at least remotely possible that messing about with
ovs-dpctl on a datapath could cause some kind of race that would
result in two entries for the same ODP port number in the internal
map, and I'd rather catch that early.
In ofproto-dpif.c, in port_del(), it might be a good idea to avoid
calling dpif_port_del() if ofp_port_to_odp_port() returns OVSP_NONE.
Should port_dump_next() skip and warn about ports for which
odp_port_to_ofp_port() returns OFPP_NONE?
ofp_port_to_odp_in_port() seems really unneeded now, since there
should be no port numbered OFPP_CONTROLLER or OFPP_NONE.
I'm a little uncomfortable with this but I can't quite put a finger on
the issue, so I'll defer further review of it for the moment.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
