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

Ben Pfaff blp at nicira.com
Sat Jan 4 07:21:18 UTC 2014


Hi Neil. I'm on vacation until next Monday, so I'll read your message in
detail and get back to you sometime next week. Sorry for the delay.
On Jan 3, 2014 7:39 PM, "Neil McKee" <neil.mckee at inmon.com> wrote:

>
> On Mon, Dec 30, 2013 at 4:08 PM, Ben Pfaff <blp at nicira.com> wrote:
>
>> On Fri, Dec 20, 2013 at 03:45:37PM -0800, Neil McKee wrote:
>> > Sorry.  Grappling with a new email client.  Here it as an attachment, if
>> > that's OK.  Let me know if I need to rebase again.
>>
>> This patch provokes one warning from GCC (and Clang):
>>
>>     ../lib/odp-util.c: In function 'parse_odp_action':
>>     ../lib/odp-util.c:515:27: error: format '%lli' expects argument of
>> type 'long long int *', but argument 3 has type 'uint32_t *'
>> [-Werror=format]
>>
>> (It looks like you want %"SCNi32" instead of %lli.)
>>
>> This patch provokes several warnings from "sparse":
>>
>>     ../ofproto/ofproto-dpif-sflow.c:156:31: warning: cast to restricted
>> ofp_port_t
>>     ../ofproto/ofproto-dpif-sflow.c:405:44: warning: cast from restricted
>> ofp_port_t
>>     ../ofproto/ofproto-dpif-sflow.c:431:68: warning: cast from restricted
>> ofp_port_t
>>     ../tests/test-sflow.c:255:19: warning: incorrect type in assignment
>> (different base types)
>>     ../tests/test-sflow.c:255:19:    expected unsigned int [unsigned]
>> [usertype] all
>>     ../tests/test-sflow.c:255:19:    got restricted __be32
>>     ../tests/test-sflow.c:308:9: warning: incorrect type in assignment
>> (different base types)
>>     ../tests/test-sflow.c:308:9:    expected unsigned int [unsigned]
>> [usertype] src
>>     ../tests/test-sflow.c:308:9:    got restricted __be32
>>     ../tests/test-sflow.c:309:9: warning: incorrect type in assignment
>> (different base types)
>>     ../tests/test-sflow.c:309:9:    expected unsigned int [unsigned]
>> [usertype] dst
>>     ../tests/test-sflow.c:309:9:    got restricted __be32
>>     ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:310:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:310:45:    got unsigned int [unsigned]
>> [usertype] src
>>     ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:310:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:310:45:    got unsigned int [unsigned]
>> [usertype] src
>>     ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:310:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:310:45:    got unsigned int [unsigned]
>> [usertype] src
>>     ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:310:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:310:45:    got unsigned int [unsigned]
>> [usertype] src
>>     ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:311:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:311:45:    got unsigned int [unsigned]
>> [usertype] dst
>>     ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:311:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:311:45:    got unsigned int [unsigned]
>> [usertype] dst
>>     ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:311:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:311:45:    got unsigned int [unsigned]
>> [usertype] dst
>>     ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1
>> (different base types)
>>     ../tests/test-sflow.c:311:45:    expected restricted __be32
>> [usertype] x
>>     ../tests/test-sflow.c:311:45:    got unsigned int [unsigned]
>> [usertype] dst
>>     ../ofproto/ofproto-dpif.c:1495:49: warning: incorrect type in
>> argument 2 (different base types)
>>     ../ofproto/ofproto-dpif.c:1495:49:    expected restricted ofp_port_t
>> [usertype] ofp_port
>>     ../ofproto/ofproto-dpif.c:1495:49:    got restricted odp_port_t
>> [usertype] odp_port
>>     ../ofproto/ofproto-dpif-sflow.c:623:33: error: undefined identifier
>> 'IPPROTO_GRE'
>>     ../ofproto/ofproto-dpif-sflow.c:631:33: error: undefined identifier
>> 'IPPROTO_ESP'
>>
>> (The two just above mean that we need to add IPPROTO_* declarations to
>> include/sparse/netinet/in.h.)
>>
>> Please limit lines to 79 characters.
>>
>> Please put a space after the 'if' keyword.
>>
>> I'm not sure why this adds an unnamed sub-struct to struct slave in
>> lacp.c.  I'd just put those members right in struct slave.
>>
>> In parse_odp_action(), please use ovs_scanf() instead of sscanf(), as
>> the existing code does.
>>
>> In a couple of places,
>>             } else {
>> instead of
>>             }
>>             else {
>>
>> In union user_action_cookie, padding should not be necessary.  (I see
>> that there is an obsolete comment there; I'll fix it.)
>>
>> Also in union user_action_cookie, the doubly nested union looks odd to
>> me.  Let's just use one level.  You can name the outer member
>> sflow_single and sflow_multi, or some other appropriate names.
>>
>> In ofproto/ofproto-dpif-sflow.c, please write
>>         #include "byte-order.h"
>> instead of
>>         #include "lib/byte-order.h".
>> (I see that there's a bad example in that file; I'll fix it.)
>> Also, please keep the list of #includes in alphabetical order.
>>
>>
> OK to all that.
>
>
>> 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)?
>
> I'm pretty nervous about all the code added at the end of
>> ofproto-dpif-sflow.c that checks netdev types based on strings, etc.
>> It seems brittle: no one is going to remember to update it.  It would
>> be better to integrate it into the netdev code by adding appropriate
>> netdev_*() functions for returning the information you need (such as
>> the IP protocol used for tunneling, the ports, the keys, and so on).
>> Did you consider that?
>>
>>
> Agreed.  I just try to avoid making changes to files that don't match
> *sflow*.  I confess I was half-expecting you to refactor all that
> yourself,  but I'm happy to have a go at it once I understand the
> callback issue above.
>
> Neil
>
>
>
>> Thanks,
>>
>> Ben.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140103/a23b3406/attachment-0004.html>


More information about the dev mailing list