[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