flow/manager: fix flows not evicted & freed in time

Flows have been shown to linger for a long time w/o giving up their
resources. This would lead to higher memory use and memcaps getting
reached.

Three main causes have been identified:

Slow passes hash passes. By default the flow manager will scan the
flow hash slowly. It is based on the flow timeout settings, and with
the default config it will take 4 minutes for a full scan to be
complete. This leaves a window for flows that are timed out to linger
for minutes longer than expected.

Flow Manager yields under pressure. The per row TryLock causes work
to be delayed more. The Flow manager will use trylock on a hash row
and will yield immediately if the row is busy. This means that it will
take a full pass before the row is revisited again. If the row holds
busy flows, this could happen many times in a row.

Flow Manager favors evicted flows over active flows. The Flow Manager
will only process the evicted flows if they are present. These flows
have been evicted by workers. The active flows on that hash row will
have to wait until the next hash pass. Of course by then there could
be more evicted flows.

Combined these factors could lead to flows not being considered for
freeing and logging for a very long time, potentially even indefinitly.

The patch addresses the latter two flow manager issues by no longer
using TryLock. It will now simply wait for the lock to be released and
then do its work on it. Additionally for each row both the evicted list
and the active flow list will be processed.

Bug: #4650.

diff --git a/src/flow-manager.c b/src/flow-manager.c
index 7d1b296e0662e060d4a43ce65036fd1d288033e0..1ddb21fcc2efdc19306780bedd61177993572abc 100644 (file)
--- a/src/flow-manager.c
+++ b/src/flow-manager.c
@@ -116,7 +116,6 @@ typedef struct FlowTimeoutCounters_ {
     uint32_t rows_checked;
     uint32_t rows_skipped;
     uint32_t rows_empty;
-    uint32_t rows_busy;
     uint32_t rows_maxlen;
 
     uint32_t flows_checked;
@@ -265,11 +264,6 @@ static inline int FlowBypassedTimeout(Flow *f, struct timeval *ts,
     return 1;
 }
 
-static inline int FMTryLockBucket(FlowBucket *fb)
-{
-    int r = FBLOCK_TRYLOCK(fb);
-    return r;
-}
 static inline void FMFlowLock(Flow *f)
 {
     FLOWLOCK_WRLOCK(f);
@@ -420,7 +414,6 @@ static uint32_t FlowTimeoutHash(FlowManagerTimeoutThread *td,
     const int emergency = ((SC_ATOMIC_GET(flow_flags) & FLOW_EMERGENCY));
     const uint32_t rows_checked = hash_max - hash_min;
     uint32_t rows_skipped = 0;
-    uint32_t rows_busy = 0;
     uint32_t rows_empty = 0;
 
 #if __WORDSIZE==64
@@ -444,43 +437,33 @@ static uint32_t FlowTimeoutHash(FlowManagerTimeoutThread *td,
         for (uint32_t i = 0; i < check; i++) {
             FlowBucket *fb = &flow_hash[idx+i];
             if ((check_bits & ((TYPE)1 << (TYPE)i)) != 0 && SC_ATOMIC_GET(fb->next_ts) <= (int32_t)ts->tv_sec) {
-                if (FMTryLockBucket(fb) == 0) {
-                    Flow *evicted = NULL;
-                    if (fb->evicted != NULL || fb->head != NULL) {
-                        /* if evicted is set, we only process that list right now.
-                         * Since its set we've had traffic that touched this row
-                         * very recently, and there is a good chance more of it will
-                         * come in in the near future. So unlock the row asap and leave
-                         * the possible eviction of flows to the packet lookup path. */
-                        if (fb->evicted != NULL) {
-                            /* transfer out of bucket so we can do additional work outside
-                             * of the bucket lock */
-                            evicted = fb->evicted;
-                            fb->evicted = NULL;
-                        } else if (fb->head != NULL) {
-                            int32_t next_ts = 0;
-                            FlowManagerHashRowTimeout(td,
-                                    fb->head, ts, emergency, counters, &next_ts);
-
-                            if (SC_ATOMIC_GET(fb->next_ts) != next_ts)
-                                SC_ATOMIC_SET(fb->next_ts, next_ts);
-                        }
-                        if (fb->evicted == NULL && fb->head == NULL) {
-                            SC_ATOMIC_SET(fb->next_ts, INT_MAX);
-                        } else if (fb->evicted != NULL && fb->head == NULL) {
-                            SC_ATOMIC_SET(fb->next_ts, 0);
-                        }
-                    } else {
-                        SC_ATOMIC_SET(fb->next_ts, INT_MAX);
-                        rows_empty++;
+                FBLOCK_LOCK(fb);
+                Flow *evicted = NULL;
+                if (fb->evicted != NULL || fb->head != NULL) {
+                    if (fb->evicted != NULL) {
+                        /* transfer out of bucket so we can do additional work outside
+                         * of the bucket lock */
+                        evicted = fb->evicted;
+                        fb->evicted = NULL;
                     }
-                    FBLOCK_UNLOCK(fb);
-                    /* processed evicted list */
-                    if (evicted) {
-                        FlowManagerHashRowClearEvictedList(td, evicted, ts, counters);
+                    if (fb->head != NULL) {
+                        int32_t next_ts = 0;
+                        FlowManagerHashRowTimeout(td, fb->head, ts, emergency, counters, &next_ts);
+
+                        if (SC_ATOMIC_GET(fb->next_ts) != next_ts)
+                            SC_ATOMIC_SET(fb->next_ts, next_ts);
+                    }
+                    if (fb->evicted == NULL && fb->head == NULL) {
+                        SC_ATOMIC_SET(fb->next_ts, INT_MAX);
                     }
                 } else {
-                    rows_busy++;
+                    SC_ATOMIC_SET(fb->next_ts, INT_MAX);
+                    rows_empty++;
+                }
+                FBLOCK_UNLOCK(fb);
+                /* processed evicted list */
+                if (evicted) {
+                    FlowManagerHashRowClearEvictedList(td, evicted, ts, counters);
                 }
             } else {
                 rows_skipped++;
@@ -493,7 +476,6 @@ static uint32_t FlowTimeoutHash(FlowManagerTimeoutThread *td,
 
     counters->rows_checked += rows_checked;
     counters->rows_skipped += rows_skipped;
-    counters->rows_busy += rows_busy;
     counters->rows_empty += rows_empty;
 
     if (td->aside_queue.len) {
@@ -851,7 +833,7 @@ static TmEcode FlowManager(ThreadVars *th_v, void *thread_data)
             const uint32_t secs_passed = rt - flow_last_sec;
 
             /* try to time out flows */
-            FlowTimeoutCounters counters = { 0, 0, 0, 0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
+            FlowTimeoutCounters counters = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
 
             if (emerg) {
                 /* in emergency mode, do a full pass of the hash table */