Thanks for the detailed explanation. You should teach a class :) I'll rework the patch.
Neil ------ Neil McKee InMon Corp. http://www.inmon.com On Thu, Jan 9, 2014 at 11:43 AM, Ben Pfaff <b...@nicira.com> wrote: > On Fri, Jan 03, 2014 at 09:39:00PM -0800, Neil McKee wrote: > > On Mon, Dec 30, 2013 at 4:08 PM, Ben Pfaff <b...@nicira.com> wrote: > > > I'm not a fan of callbacks because of the surprising inversion of > > > control that they cause, and especially not of layered callbacks like > > > the one that this introduces. Instead of calling back into > > > dpif_sflow_port_lookup_callback() from sflow_agent_get_counters(), I > > > think that it would be better to provide the necessary information in > > > the call to dpif_sflow_add_port(). > > > > > > > > I wasn't expecting you to say that. The purpose of the callback is > > really just to access the state held within ofproto-dpif, but in an > > encapsulated way so that the exposure is limited and controlled. > > The alternatives were (1) to make those private structure fields > > visible to the ofproto-dpif-sflow module so it could just read them > > directly, or (2) to copy all the relevant state into a separate > > hash-table just for ofproto-dpif-sflow to use when processing packet > > samples. It sounds like you prefer (2)? To understand why I think > > I just need to know what you mean by "surprising inversion of control". > > Are you referring to the possibility of mutex contention/deadlock > > if the data-structures within ofproto-dpif are interrogated from > > within the packet-processing path (i.e. when processing an > > sFlow packet sample)? > > Let me step back and explain, since my phrasing didn't help. I'm > going to explain one of my own views of program structure. Most of it > is opinion, not fact, so bear with me a little. > > The call stack in a well-structured large program usually follows a > tree structure from one logical module (often, one source file) to > another. Often, you can consider one module to be "higher level" or > "lower level" than another based on the rule that higher level modules > call into lower level modules, but not vice versa. Sometimes calls go > both ways, and in my experience that is undesirable because it makes > the structure of the program harder to understand. Direct calls are > one thing, because it is easy enough to trace through them with "grep" > and other tools, but indirect calls through callbacks are harder to > understand. Actually, there are at least two classes of callbacks: > callbacks that you pass into a function for that function to call, and > callbacks that you store somewhere in data owned by a module that in > theory any function in that module *could* call (even if it doesn't > currently). The latter are harder to understand. > > My personal idea of nightmarish tangles of callbacks is GObject and in > particular GTK+. Everything is a callback and it's very difficult to > figure out exactly when one of them could get called. You end up > writing bad code because you can't figure it out. (GTK+ has other > problems too, that's just one of them.) > > Looking at the callback in question again, it's going to be difficult > to maintain for a reason that you basically called out in the comment, > that is, locking. In most places in OVS we are able to use Clang > thread safety annotations to statically check (to some basic level of > safety, anyhow) that locks are held or not held at various places. > But those checks can't flow through callback functions: > > /* XXX do we need to acquire ofproto_mutex here? Or should it be > acquired in handle_sflow_upcall() below, > * so that it will not be released until after the sFlow-module has > followed the pointers we are giving > * it here and the sflow sample has been fully processed? */ > > So I'd prefer, if we can avoid it, to use some interface more direct > than a callback. >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
