[ovs-dev] [PATCH] sFlow LAG and Tunnel export

[Neil McKee]

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 <blp at 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 <blp at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140113/d11e0096/attachment-0003.html>