Skip to content

Commit

Permalink
threading: don't pass locked flow between threads
Browse files Browse the repository at this point in the history
Previously the flow manager would share evicted flows with the workers
while keeping the flows mutex locked. This reduced the number of unlock/
lock cycles while there was guaranteed to be no contention.

This turns out to be undefined behavior. A lock is supposed to be locked
and unlocked from the same thread. It appears that FreeBSD is stricter on
this than Linux.

This patch addresses the issue by unlocking before handing a flow off
to another thread, and locking again from the new thread.

Issue was reported and largely analyzed by Bill Meeks.

Bug: OISF#4478
(cherry picked from commit 9551cd0)
  • Loading branch information
victorjulien committed Sep 6, 2021
1 parent 2ce308f commit b5e01be
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 1 deletion.
1 change: 1 addition & 0 deletions src/flow-hash.c
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ static inline void MoveToWorkQueue(ThreadVars *tv, FlowLookupStruct *fls,
f->fb = NULL;
f->next = NULL;
FlowQueuePrivateAppendFlow(&fls->work_queue, f);
FLOWLOCK_UNLOCK(f);
} else {
/* implied: TCP but our thread does not own it. So set it
* aside for the Flow Manager to pick it up. */
Expand Down
2 changes: 1 addition & 1 deletion src/flow-manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -332,9 +332,9 @@ static uint32_t ProcessAsideQueue(FlowManagerTimeoutThread *td, FlowTimeoutCount
FlowForceReassemblyNeedReassembly(f) == 1) {
/* Send the flow to its thread */
FlowForceReassemblyForFlow(f);
FLOWLOCK_UNLOCK(f);
/* flow ownership is passed to the worker thread */

/* flow remains locked */
counters->flows_aside_needs_work++;
continue;
}
Expand Down
1 change: 1 addition & 0 deletions src/flow-timeout.c
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ static inline void FlowForceReassemblyForHash(void)
RemoveFromHash(f, prev_f);
f->flow_end_flags |= FLOW_END_FLAG_SHUTDOWN;
FlowForceReassemblyForFlow(f);
FLOWLOCK_UNLOCK(f);
f = next_f;
continue;
}
Expand Down
1 change: 1 addition & 0 deletions src/flow-worker.c
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ static void CheckWorkQueue(ThreadVars *tv, FlowWorkerThreadData *fw,
{
Flow *f;
while ((f = FlowQueuePrivateGetFromTop(fq)) != NULL) {
FLOWLOCK_WRLOCK(f);
f->flow_end_flags |= FLOW_END_FLAG_TIMEOUT; //TODO emerg

const FlowStateType state = f->flow_state;
Expand Down

0 comments on commit b5e01be

Please sign in to comment.