From 244dd11c34152fbdb01636da8a9fa4a9fa0de050 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sun, 7 Nov 2021 06:25:31 +0100 Subject: [PATCH] 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. --- src/flow-manager.c | 68 +++++++++++++++++----------------------------- 1 file changed, 25 insertions(+), 43 deletions(-) diff --git a/src/flow-manager.c b/src/flow-manager.c index 7d1b296e06..1ddb21fcc2 100644 --- 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 */ -- 2.47.2