dpdk: use DPDK EAL threads for workers v5.1#14691
dpdk: use DPDK EAL threads for workers v5.1#14691
Conversation
As the next step in the current implementation after pthread_exit was to end the thread execution anyway.
Lcore initialization is needed to leverage additional DPDK features, e.g., packet mempool per-core cache. Threads are loaded from the CPU affinity of active interfaces, meaning, interfaces which are defined under dpdk.interfaces list. The list is superset of actually used cores. This means that DPDK EAL init may create more lcores than actually needed. It may happen when defining affinity for e.g. "all" but the device/interface only using 4 threads. Additional consideration should be on autopin which can pick cores from the cpu affinity list non-sequentially but rather according to NUMA mapping. Lcore super set is not known to introduce any functional or performance problem. Ticket: 8084
As a prerequisite for the upcoming DPDK thread management, this commit allows custom spawn and join (wait) thread functions to be configured. Ticket: 8084
This change leverages DPDK functions from rte_eal* for launching worker function on the selected DPDK lcores. Ticket: 8084
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14691 +/- ##
==========================================
- Coverage 82.13% 82.11% -0.03%
==========================================
Files 1011 1012 +1
Lines 262922 263101 +179
==========================================
+ Hits 215956 216033 +77
- Misses 46966 47068 +102
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline = 29272 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tv->thread_id = AffinityGetNextCPU(tv, taf); | ||
|
|
||
| SCLogPerf("Setting prio %d for thread \"%s\" to cpu/core " | ||
| "%" PRIu64 ", thread id %lu", |
There was a problem hiding this comment.
The log message uses SCGetThreadIdLong() which returns the calling thread's ID (the main thread spawning the worker), not the DPDK worker thread's ID. Since this log is printed before the DPDK thread is actually launched, it will show the ID of the spawning thread rather than the worker thread's ID. Consider either removing the thread id %lu part or clarifying in the message that this is the spawning thread's ID, not the worker thread's ID.
| "%" PRIu64 ", thread id %lu", | |
| "%" PRIu64 ", spawning thread id (current thread) %lu", |
| if (CPU_ISSET(tv->thread_id, &taf->lowprio_cpu)) { | ||
| tv->thread_priority = PRIO_LOW; | ||
| } else if (CPU_ISSET(tv->thread_id, &taf->medprio_cpu)) { | ||
| tv->thread_priority = PRIO_MEDIUM; | ||
| } else if (CPU_ISSET(tv->thread_id, &taf->hiprio_cpu)) { | ||
| tv->thread_priority = PRIO_HIGH; | ||
| } else { | ||
| tv->thread_priority = taf->prio; | ||
| } |
There was a problem hiding this comment.
The priority is being determined using tv->thread_id on lines 88-93, but tv->thread_id is not set until line 100. This means the CPU_ISSET checks are operating on an uninitialized or stale value of tv->thread_id. The priority determination logic should either use a different variable or be moved after tv->thread_id is assigned on line 100.
victorjulien
left a comment
There was a problem hiding this comment.
Just looked at the abstraction for now, but that will need some work. Plus some questions inline.
| return TM_ECODE_OK; | ||
| } | ||
|
|
||
| static void TmThreadSpawnPthread(ThreadVars *tv) |
There was a problem hiding this comment.
I would expect this to live in a new file tm-threads-pthreads.c or similar.
There was a problem hiding this comment.
by default, I assumed pthread can stay at the core of the code. could be extracted np.
|
|
||
| /** \brief Per thread variable structure */ | ||
| typedef struct ThreadVars_ { | ||
| pthread_t t; |
There was a problem hiding this comment.
pthread_t is an opaque type that we can't assume to fit in a uint64_t
| * this thread. It is passed directly to pthread_create(), hence the | ||
| * void pointers in and out. */ | ||
| void *(*tm_func)(void *); | ||
| void (*tm_spawn)(struct ThreadVars_ *); |
There was a problem hiding this comment.
why are these in ThreadVars and not a global thread API table of sorts?
There was a problem hiding this comment.
I believe the answer is in
why do we need these in a TmModule instead of globally?
thread
| typedef struct TmModule_ { | ||
| const char *name; | ||
|
|
||
| /** thread management, if unspecified, falls back to pthreads */ |
There was a problem hiding this comment.
why do we need these in a TmModule instead of globally?
There was a problem hiding this comment.
For the DPDK capture method, we only need to create DPDK lcores for worker threads.
There was a problem hiding this comment.
It is mainly because a single DPDK lcore is not intended to run multiple separate threads.
And that is a common case for, e.g., flow manager/recycler threads.
There was a problem hiding this comment.
so the dpdk threading support is only a partial replacement for the pthreads inside a single process? Some threads use the dpdk logic, other pure pthreads?
There was a problem hiding this comment.
correct. DPDK threads still use pthreads at the core so they are compatible but they should be manipulated through DPDK calls ideally.
There was a problem hiding this comment.
With regards to global setting, we could have thread management callbacks on the level of thread families so it does not need to be stored within ThreadVars directly and repeatedly.
Continuation of #14619
Link to ticket: https://redmine.openinfosecfoundation.org/issues/8084
Describe changes:
DPDK EAL threads are a construct in DPDK that envelopes the classic pthreads. DPDK EAL-native threads, though, offer some advantages - one of which is the use of mempool caches or the ability to run as a secondary process.
The initialization is automatic and leverages the already defined threading affinity from which it composes a list of lcores (DPDK EAL parameter "-l"). Based on this list of CPU cores, the DPDK EAL init function then initializes the threads. Suricata, during the startup, later launches worker functions on these threads.
v5:
v4 (public initial work):
Automated testing is constrained at the moment.