[ovs-dev] [upcall 2/2] ofproto-dpif: Factor out flow miss handling.

Ben Pfaff blp at nicira.com
Tue Jul 16 21:56:09 UTC 2013


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 <ethan at 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.




More information about the dev mailing list