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
