On Thu, Nov 01, 2012 at 12:23:40AM -0700, Justin Pettit wrote:
> On Oct 23, 2012, at 11:18 AM, Ben Pfaff <b...@nicira.com> wrote:
> > 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".
You're undoubtedly building 64-bit then. On 64-bit, size_t is an
alias for "unsigned long long". I'm building for 32-bit, where size_t
is an alias for "unsigned int" or "unsigned long" (can't be bothered
to check), which in any case differs from "unsigned long long", thus
the warning.
> > 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.
Thanks.
> > 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.
That's great, thank you.
> > 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.
We can defer it. Thanks.
> 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?
I will, thanks for the heads-up.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
