On 09/18/2017 03:01 AM, Weglicki, MichalX wrote:
Hi Greg - comments inline marked [MW].
-----Original Message-----
From: Greg Rose [mailto:[email protected]]
Sent: Saturday, September 16, 2017 12:45 AM
To: Weglicki, MichalX <[email protected]>
Cc: [email protected]; Darrell Ball <[email protected]>
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
