[ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key

Greg Rose gvrose8192 at gmail.com
Mon Sep 18 21:43:47 UTC 2017


On 09/18/2017 03:01 AM, Weglicki, MichalX wrote:
> Hi Greg - comments inline marked [MW].
> 
>> -----Original Message-----
>> From: Greg Rose [mailto:gvrose8192 at gmail.com]
>> Sent: Saturday, September 16, 2017 12:45 AM
>> To: Weglicki, MichalX <michalx.weglicki at intel.com>
>> Cc: dev at openvswitch.org; Darrell Ball <dball at vmware.com>
>> Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
>>
>> On 09/08/2017 06:35 AM, Weglicki, MichalX wrote:
>>> Greg,
>>>
>>> Patch is rebased and sent to mailing list as V3 (Last patch was supposed to be V2 - Przemek by accident sent it again as V1).
>>>
>>> Br,
>>> Michal.
>>
>> Michal,
>>
>> I'm not sure what has happened but I can't find your V3 patches in my mail but they are in patchwork.
>>
>> I tested the patches and they seem to work fine, or at least the code executes and I didn't see any serious
>> regression.  My ntopng/nprobe setup accepted the new template.
>>
>> However, I am seeing this now:
>>
>> 5/Sep/2017 15:11:16 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : expected string, got nil
>> 15/Sep/2017 15:11:16 [Lua.cpp:6658] WARNING: Script failure
>> [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index
>> global
>> 'stats' (a nil value)]
>> 15/Sep/2017 15:11:16 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : expected string, got nil
>> 15/Sep/2017 15:11:15 [Lua.cpp:6658] WARNING: Script failure
>> [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index
>> global
>> 'stats' (a nil value)]
>> 15/Sep/2017 15:11:15 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : expected string, got nil
>> 15/Sep/2017 15:11:12 [Lua.cpp:6658] WARNING: Script failure
>> [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index
>> global
>> 'stats' (a nil value)]
>> 15/Sep/2017 15:11:12 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : expected string, got nil
>> 15/Sep/2017 15:11:09 [Lua.cpp:6658] WARNING: Script failure
>> [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index
>> global
>> 'stats' (a nil value)]
>> 15/Sep/2017 15:11:09 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : expected string, got nil
>> 15/Sep/2017 15:11:09 [Lua.cpp:6658] WARNING: Script failure
>> [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index
>> global
>> 'stats' (a nil value)]
>> 15/Sep/2017 15:11:09 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : expected string, got nil
> [MW] Well, are you sure that this patch is causing this error message? I mean, you don't see
> It without this patch, do you? If yes, could you explain your setup a little bit so I can reproduce it here?

It took a while but I was able to reproduce without your patches applied.  Seems to be something that
occasionally occurs soon after the ntopng/nprobe services are restarted and then it doesn't occur
again SFAICT.


> 
>>
>> Also, in patch 1/2 though the interface is hard coded to Ethernet in this bit of the patch:
>>
>> +    /* Once DPDK library supports retrieving ifType we should get this value
>> +     * directly from DPDK rather than hardcoding it. */
>> +    smap_add_format(args, "if_type", "%"PRIu32, IF_TYPE_ETHERNETCSMACD);
>> +    smap_add_format(args, "if_descr", "%s %s", rte_version(),
>> +                                               dev_info.driver_name);
>>
>> I'd like to get Darrel Ball's take on this so I've CC'd him.
>>
>> In patch 2/2 I have some other comments.
> [MW] There are some DPDK patches which expose this information for the driver, nevertheless I think that anyway value "6"
> Will be returned anyway - as this is very general category for Ethernet devices. I could be improved in the future of course
> When patch will be available.
> 
>>
>> Here you change the ports to point to all ports rather than tunnel ports
>>
>> -    struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports" hmap. */
>> +    struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
>>        struct ofport *ofport;      /* To retrieve port stats. */
>>        odp_port_t odp_port;
>>        enum dpif_ipfix_tunnel_type tunnel_type;
>>        uint8_t tunnel_key_length;
>> +    uint32_t ifindex;
>>
>> I didn't see any reason this can't be done but I'm worried about side effects. Are we sure that
>> there are no other assumptions in the code that depend on that hmap pointing to only tunnel ports?
>> I realize that patch 2/2 seems to fix that up and I didn't see any particular reason to
>> doubt but I'm rather new to the code base so I thought I'd ask.
> [MW] I'm not quite sure wat do you mean here, in previous implementation only tunnel ports were
> Mandatory to cache, however currently we need all ports, as we need to query particular
> Netdev to get information that we need. I don't remember exactly the details right now, but we found
> That there is no need to cache tunnel ports separately, but it could be handled through generic list
> Of all ports.

OK, that sounds fine to me.  As I had stated, I'm new to this code base so I just wanted
to make sure that due diligence with the change had been done.  Thank you!

> 
>>
>> Further on I see this:
>>
>> +static enum dpif_ipfix_tunnel_type
>> +dpif_ipfix_tunnel_type(const struct ofport *ofport) {
>>
>> I think the opening curly brace should be on the next line down?  I saw at least one other instance
>> if this.  It passes checkpatch to put the curly brace on the same line but all other functions in
>> the source module put the opening curly brace of a function on the next line.
> [MW] True!
> 
>>
>> There are a couple of other checkpatch warnings and an error as well:
>>
>> ERROR: Too many signoffs; are you missing Co-authored-by lines?
>> WARNING: Line lacks whitespace around operator
>> #80 FILE: ofproto/ofproto-dpif-ipfix.c:317:
>>       ovs_be32 if_index;     /* (INGRESS|EGRESS)_INTERFACE */
>>
>> WARNING: Line lacks whitespace around operator
>> #81 FILE: ofproto/ofproto-dpif-ipfix.c:318:
>>       ovs_be32 if_type;     /* (INGRESS|EGRESS)_INTERFACE_TYPE */
>>
>> Lines checked: 762, Warnings: 2, Errors: 1
> [MW] Sorry about that, it is my rebase patch which caused this, I will fix it in next revision.
> 
>>
>> The rest of patch 2/2 looks OK but I am worried about the ntop errors listed above.
>> I haven't seen those before so I think they're related to this patch.
> [MW] I think that everything except those ntop errors is clear, so I'm waiting for your reply,
> After we will figure it out, I will create new patch version!

Sure, go ahead.

Thanks for your work!

- Greg



More information about the dev mailing list