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 <rleng...@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.
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"
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;
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 see that this patch adds multiple consecutive blank lines in a few
places. We don't usually do that.
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).
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.
70 minutes is a curious number to choose as the maximum for
cache_active_timeout. Is this the maximum value allowed by IPFIX?
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
