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

[Neil McKee]

Finally getting back to this.  Can we do it one piece at a time?  For
example,  attached is a patch that just adds counters to the LACP
module and a call to retrieve them.

Neil
------
Neil McKee
InMon Corp.
http://www.inmon.com


On Mon, Jan 13, 2014 at 8:29 AM, Neil McKee <neil.mckee at inmon.com> wrote:
> 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 --------------
A non-text attachment was scrubbed...
Name: 0001-Add-lacp-couters-and-a-call-to-retrieve-them.patch
Type: application/octet-stream
Size: 4816 bytes
Desc: not available
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140219/babc0c75/attachment-0005.obj>