[ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter

Romain Lenglet rlenglet at vmware.com
Tue Aug 20 21:34:40 UTC 2013


Hi Ben,

Thanks for your review!

My comments inline.
I'll send a revised patch shortly for review.

----- Original Message -----
> From: "Ben Pfaff" <blp at nicira.com>
> To: "Romain Lenglet" <rlenglet at vmware.com>
> Cc: dev at openvswitch.org
> Sent: Tuesday, August 20, 2013 1:25:16 PM
> Subject: Re: [PATCH] ipfix: implement flow caching and aggregation in exporter
> 
> On Mon, Aug 12, 2013 at 11:34:57AM -0700, Romain Lenglet wrote:
> > Implement a per-exporter flow cache with active timeout expiration.
> > Add columns "cache_active_timeout" and "cache_max_flows" into table
> > "IPFIX" to configure each cache.
> > 
> > Add per-flow elements "octetDeltaSumOfSquares",
> > "minimumIpTotalLength", and "maximumIpTotalLength" to replace
> > "ethernetTotalLength".  Add per-flow element "flowEndReason" to
> > indicate whether a flow has expired because of an active timeout, the
> > cache size limit being reached, or the exporter being stopped.
> > 
> > Signed-off-by: Romain Lenglet <rlenglet at vmware.com>
> 
> Thanks for the patch.  Sorry that it's taken so long to look at this.
> 
> "clang" reports:
> 
>     ../ofproto/ofproto-dpif-ipfix.c:1456:58: error: variable
>     'next_timeout_msec' is uninitialized when used here
>     [-Werror,-Wuninitialized]
>                 && (!has_next_timeout || temp_timeout_msec <
>                 next_timeout_msec)) {
>                                                              ^~~~~~~~~~~~~~~~~
>     ../ofproto/ofproto-dpif-ipfix.c:1448:36: note: initialize the variable
>     'next_timeout_msec' to silence this warning
>         long long int next_timeout_msec, temp_timeout_msec;
>                                        ^
>                                         = 0
> 
> You could make it happy, and simplify the code, by initializing
> next_timeout_msec to LLONG_MAX.  Or you could call
> poll_timer_wait_until(), which is cheap, each place you update
> next_timeout_msec.

Done.  I'm going with the latter solution, it's nicer.

> Please include <sys/time.h> just after "ofproto-dpif-ipfix.h" rather
> than just before.  That way, we can be sure that ofproto-dpif-ipfix.h
> remains self-contained:
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index 8e8e7a2..c37da5f 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -15,15 +15,18 @@
> >   */
> >  
> >  #include <config.h>
> > +#include <sys/time.h>
> >  #include "ofproto-dpif-ipfix.h"

Done.

> The reason for this change isn't obvious.  But we cannot use
> __kernel_time_t because it is not a portable type (it is Linux kernel
> specific):
> > @@ -41,7 +44,11 @@
> >  struct dpif_ipfix_exporter {
> >      struct collectors *collectors;
> >      uint32_t seq_number;
> > -    time_t last_template_set_time;
> > +    __kernel_time_t last_template_set_time;

Done, reverted this change.

> Perhaps on the same topic, this is the first time I've run into
> timercmp().  It appears to be a nonstandard BSDism.  Is there a reason
> to use "struct timeval" instead of the "long long int as msecs from
> the epoch" that we usually use in OVS?  The latter is nice, when you
> can use it, because arithmetic and relational operators are meaningful
> and simple.

I send microsecond-granularity timestamps in the IPFIX flow records, and
only timeval provides values at that granularity.
Even though it doesn't matter much right now since timestamping is done
in userspace and is imprecise, I'd like to be able to support precise
(e.g. hardware, or at least kernelspace) timestamping later.

> I see that this patch adds multiple consecutive blank lines in a few
> places.  We don't usually do that.

Done.

> In ipfix_cache_next_timeout_msec(), usually I put a space just after
> LIST_FOR_EACH (because I want it to look like a statement, not a
> function call).

Done.

> I see that ipfix_cache_entry_init() uses ofpbuf_use_stub() and then
> later asserts that the buffer was not reallocated.  If that is the
> behavior that you want, then you can use ofpbuf_use_stack(), which
> will assert-fail immediately when reallocation is attempted.

Done.  Yes, that's the behavior I want and I switched to using
ofpbuf_use_stack().  Thanks for the suggestion.

I've also redefined the flow_key_msg_part buffer member to be a
uint64_t array to ensure proper alignment.

> 70 minutes is a curious number to choose as the maximum for
> cache_active_timeout.  Is this the maximum value allowed by IPFIX?

This is because I send timestamps as unsigned 32-bit deltas in
microseconds.  This corresponds to ~71.5 minutes.
That's a limit in the standard:
http://tools.ietf.org/html/rfc5102#section-5.9

That constraint on the timestamps directly translates into a
constraint on the cache timeouts.
--
Romain Lenglet



More information about the dev mailing list