On Oct 23, 2012, at 11:18 AM, Ben Pfaff <b...@nicira.com> wrote:
> "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.
Fixed.
> 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"
I'm using Ubuntu's GCC 4.6.1, and it didn't report a warning. I've replaced it
with "%zu".
> The comment on struct dpif_backer should probably say all datapaths of
> a given type rather than just plain all data paths.
Good point.
> 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.
That is cleaner. Thanks.
> 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.
It's like you're in my head; you must have been lazy in a previous life. I'd
wanted to use sset, but didn't see a clean way to do it. Thanks for the
suggestion. I just sent out that addition to the sset library and updated my
patch to use it.
> 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.
Great point. I've now renamed it "ovs-<type>", where "<type>" is replaced by
the type.
> 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.
Done.
> 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.
Okay.
> In construct(), please put a space between SHASH_FOR_EACH_SAFE and its
> argument list.
Done.
> In port_destruct(), it seems a little odd to me that the underlying
> device might not be there. Is it likely not to be?
On ofproto destruction it is. The current code assumes that as part of the
ofproto being destroyed, the ports will go with it. That changes with our
single datapath, since a datapath is a logical construct. I've added a comment
describing this.
> This patch adds a double blank line in handle_miss_upcalls().
Fixed.
> 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.
This is being called on the ofproto, but flows are being pulled from the
backer. If you have multiple bridges, then they need to ignore the others'
flows. If this logic isn't there, it tries to delete these "unexpected" flows.
As we discussed off-line, I reimplemented the logic so that expire() is now
backer-based, which is more efficient.
> Any ideas on the XXX in show_dp_format():
> + /* xxx It would be better to show bridge-specific stats instead
> + * xxx of dp ones. */
I wasn't sure how best to get this information, so I was planning to add it
shortly to a follow-on patch, since this series is blocking some other work
that needs to get done by others. If you'd like to see it in before the code
goes in, let's talk about the best way to get the information.
> 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.
Fixed.
> Thanks a lot for doing this work!
Thank you very much for the review!
I'll send out a revised patch soon. In addition to the changes you suggested,
I added some Apple-like logic to open_dpif_backer() that assume we're the only
thing controller the kernel datapath and blows away any unexpected datapaths.
If I don't, there can be some confusion during upgrade, since we're replacing
existing datapaths with logical ones. Can you sanity-check that logic, too?
Thanks again!
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
