On Thu, Oct 18, 2012 at 12:52:00PM -0700, Justin Pettit wrote:
> This commit switches to using a single backing datapath (called
> "ovs-datapath") for all bridges of that datapath's type. Previously,
> resources couldn't be shared across bridges, since each was in its own
> datapath. This change will allow sharing of tunnels and cheaper patch
> ports to be added in the future.
>
> Since bridges share a common datapath, the ovs-dpctl commands won't
> provide bridge-specific information. Users wishing to have that
> information should use the new "ovs-appctl dpif/*" commands as
> documented in ovs-vswitchd(8).
>
> Signed-off-by: Justin Pettit <jpet...@nicira.com>
"git am" says:
Applying: ofproto-dpif: Use a single underlying datapath across multiple br\
idges.
/home/blp/ovs/.git/rebase-apply/patch:696: trailing whitespace.
return EOF;
warning: 1 line adds whitespace errors.
GCC says:
../ofproto/ofproto-dpif.c: In function "show_dp_format":
../ofproto/ofproto-dpif.c:7371: error: format "%llu" expects type "long l\
ong unsigned int", but argument 3 has type "size_t"
The comment on struct dpif_backer should probably say all datapaths of
a given type rather than just plain all datapaths.
The while loop in type_run() in ofproto-dpif.c might be slightly
easier to read if one changed the condition from:
while ((error = dpif_port_poll(backer->dpif, &devname)) != EAGAIN) {
to
while ((error = dpif_port_poll(backer->dpif, &devname)) == 0) {
and then handled the error != EAGAIN case following the loop instead
of inside it.
I don't much like using an svec instead of an sset for the "ports"
member of ofproto_dpif. It's pretty unnatural for most purposes, and
slower than an sset. Is it that way just to implement
port_dump_next()? I think that it would be better to work out
something like hmap_at_position() for an sset (I think you can just
write an sset wrapper for that, since an sset is an hmap), and then
use that to implement port iteration, instead of using an svec.
I think that, until now, ovs-vswitchd has technically been able to
switch multiple kinds of datapath (e.g. a netdev datapath and a linux
datapath) at the same time, even if hardly any users actually did
that. But the name of the backer is always ovs-datapath, regardless
of the type. Our code isn't very good at distinguishing instances
with the same name but different types, so I think that will prevent
having multiple datapath types at once. It would be better, then, if
we could choose different datapath names for different types.
It might be nice to add assert(backer->refcount > 0); at the top of
close_dpif_backer(). Occasionally that kind of assertion can find bugs.
In open_dpif_backer() the call to dpif_recv_purge() should be
unnecessary since dpif_recv_set() hasn't yet been called to enable
receiving messages. Oh, I see we were doing that in construct() too.
Well, it's still not needed.
In construct(), please put a space between SHASH_FOR_EACH_SAFE and its
argument list.
In port_destruct(), it seems a little odd to me that the underlying
device might not be there. Is it likely not to be?
This patch adds a double blank line in handle_miss_upcalls().
In update_stats(), what's the rationale for the added code:
odp_flow_key_to_flow(key, key_len, &flow);
if (odp_port_to_ofp_port(p, flow.in_port) == OFPP_NONE) {
continue;
}
This is pretty expensive and it duplicates work done in
subfacet_find(), which is called as the next line of code. All it
does is drop flows that have no port, which I'd imagine would be a
vanishingly small number of flows.
Any ideas on the XXX in show_dp_format():
+ /* xxx It would be better to show bridge-specific stats instead
+ * xxx of dp ones. */
The new comment in struct ofproto_class need a period after "datapath" here:
+ * set of ports in a datapath For this reason, the client needs a way to
+ * iterate through all the ports that are actually in a datapath. These
+ * functions provide that functionality.
Thanks a lot for doing this work!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
