On Fri, Jul 05, 2013 at 03:21:28PM -0700, Ethan Jackson wrote:
> This patch pulls flow miss handling into its own module,
> ofproto-dpif-upcall. In the short term, this new design imposes
> some additional overhead, but this is necessary to enable
> multithreading in the near future.
>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
The code looks generally good. I don't see anything to complain about
on a correctness front (though maybe I should scrutinize harder?), but
regarding readability, it could really better guide the reader,
through more careful choices of names and the addition of comments.
Some specific points:
- Maybe a paragraph or two at the top of ofproto-dpif.h to explain
the overall structure of the ofproto-dpif, and perhaps at the
top of ofproto-dpif-{xlate,upcall}.h to describe their
particular specialties.
- Function-level comments would be nice.
- Both ofproto-dpif.c and ofproto-dpif-upcall.c have functions
named handle_flow_miss(), which is confusing to the human
reader. Maybe each could be renamed to describe what it
actually does now.
- flow_miss_batch_destroy() has two variables named 'next'.
In flow_miss_batch_destroy(), the hmap_remove() call is technically
unnecessary, though I will understand if you want to keep it.
In recv_upcalls():
/* XXX: Does this malloc show up in benchmarks? If so we could
* preallocate upcalls and store them in a free list. For now,
* this is simpler. */
I wouldn't even bother with this kind of comment. In general, when
code shows up high in the profile, we'll stomp on it.
I know that the coding style says to avoid assignments in "while"
conditions, but this code in udpif_destroy() really seems to scream
out for it and I'm inclined to say we should do it:
for (drop_key = drop_key_next(udpif); drop_key;
drop_key = drop_key_next(udpif)) {
drop_key_destroy(drop_key);
}
for (upcall = upcall_next(udpif); upcall; upcall = upcall_next(udpif)) {
upcall_destroy(upcall);
}
for (fmb = flow_miss_batch_next(udpif); fmb;
fmb = flow_miss_batch_next(udpif)) {
flow_miss_batch_destroy(fmb);
}
In the comment on udpif_revalidate(), s/They caller is expect/The
caller is expected/:
+/* Notifies 'udpif' that something changed which may render previous
+ * xlate_actions() results invalid. 'fmbs' is populated with the list of
+ * flow_miss_batches which were waiting to be processed. They caller is expect
+ * to deallocate them after inspecting them. */
I don't really like how udpif_revalidate() works, but I don't have a
better suggestion.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
