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

[Weglicki, MichalX]

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? 

> 
> 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. 

> 
> 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!

> 
> Thanks for work on this!  Hopefully we can sort this all out and get your ipfix
> feature enhancement applied.
> 
> Regards,
> 
> - Greg
>