[WIP] feat: multi-thread support#2853
Draft
irozzo-1A wants to merge 127 commits intofalcosecurity:masterfrom
Draft
[WIP] feat: multi-thread support#2853irozzo-1A wants to merge 127 commits intofalcosecurity:masterfrom
irozzo-1A wants to merge 127 commits intofalcosecurity:masterfrom
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irozzo-1A The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
irozzo-1A
force-pushed
the
experiment/folly-concurrent-hashmap
branch
from
f51d695 to
e06f862
Compare
poiana
added
dco-signoff: yes
dco-signoff: no
and removed
dco-signoff: no
dco-signoff: yes
labels
irozzo-1A
force-pushed
the
experiment/folly-concurrent-hashmap
branch
from
58be2dd to
8ef1ed0
Compare
irozzo-1A
force-pushed
the
experiment/folly-concurrent-hashmap
branch
from
8c2726d to
e1dab17
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2853 +/- ##
==========================================
+ Coverage 75.21% 76.07% +0.86%
==========================================
Files 297 308 +11
Lines 32330 33636 +1306
Branches 5116 5355 +239
==========================================
+ Hits 24316 25588 +1272
- Misses 8014 8048 +34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
irozzo-1A
force-pushed
the
experiment/folly-concurrent-hashmap
branch
from
617960b to
32689a5
Compare
irozzo-1A
force-pushed
the
experiment/folly-concurrent-hashmap
branch
from
53b8b73 to
0c94fc4
Compare
|
Please double check driver/API_VERSION file. See versioning. /hold |
… path The modern-bpf engine packs both the ring-buffer index (lower 16 bits) and retry backoff state (upper 16 bits) into scap_buffer_t. Two bugs resulted from this encoding: 1. scap_buffer_next() used the full encoded handle as an array index for per-buffer event counters. Since the upper bits are always non-zero (initial retry = 500), the bounds check always failed and counters were never incremented, causing scap_event_get_num() to undercount. 2. scap_buffer_t was passed by value through the entire call chain, so SET_RETRY_US() inside the engine only modified a local copy. The exponential backoff (500us -> 30ms) never persisted across calls. Fix (1) by adding SCAP_BUFFER_INDEX() to extract the lower 16 bits for use as an array index. Fix (2) by changing next_from_buffer and scap_buffer_next to take scap_buffer_t* so the engine can mutate the caller's handle to persist retry state. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
… document get_main_thread risk In get_oldest_matching_ancestor(), the visitor lambda captured a raw sinsp_threadinfo* that could dangle after traverse_parent_state() released its shared_ptr. Under concurrent erase, reading leader->m_tid was a use-after-free. Fix by capturing the tid (int64_t) instead of the raw pointer. Also add a \warning to get_main_thread() documenting that the returned shared_ptr is a non-owning alias when `this` is the main thread, so callers must hold a separate owning reference. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…-thread Introduce a compile-time sync policy mechanism that templates the core data structures (threadinfo, fdinfo, fdtable, thread_manager) on a SyncPolicy tag, selecting either no-op or real mutexes. sinsp itself remains a plain class, using whichever policy the build selects. By default, sync_policy_default resolves to sync_policy_single (zero-cost no-op mutexes). Multi-threaded support (real mutexes, Folly ConcurrentHashMap) is activated only when building with -DENABLE_MULTI_THREAD=ON. - Add sync_policy.h with single/concurrent policies and traits - Template threadinfo, fdinfo, fdtable, thread_manager on SyncPolicy - Add factory classes for policy-aware construction - Add ENABLE_MULTI_THREAD CMake option (OFF by default) - Gate sync_policy_default on the ENABLE_MULTI_THREAD preprocessor flag Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Only compile usergroup_manager_concurrent and thread_manager_concurrent unit tests when ENABLE_MULTI_THREAD is on, matching the thread-safe sinsp build. Enable ENABLE_MULTI_THREAD alongside ENABLE_THREAD_POOL in CI (including build-libs-clang) and in test coverage so those tests run in the configuration they require. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…sion Replace the shared_mutex-guarded std::unordered_set with folly::ConcurrentHashMap for m_suppressed_tids when Folly is available. This eliminates rwlock contention on the process_event() hot path, which was consuming ~10% of CPU under multi-threaded workloads. Key changes: - Add std::atomic<bool> m_active for a fast-path early return when no comms/tids are suppressed (the common case). - Under LIBSINSP_USE_FOLLY: m_suppressed_tids lookups, inserts, and erases are entirely lock-free; a separate std::mutex (m_comms_mutex) protects only the cold-path m_suppressed_comms and m_tids_tree. - Without Folly: fall back to the original shared_mutex behavior plus the atomic fast-path. - Extract helper methods (find/insert/erase/clear/size) to abstract the two container implementations. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Remove two hot-path sources of pthread_rwlock_wrlock contention that together consumed ~4.7% of CPU in multi-threaded Falco: 1. snapshot_oldname() (2.35% CPU): replaced the per-event string copy under exclusive lock with a lightweight m_name_changed dirty flag. The flag is set by set_name()/set_name_locked() and consumed once by parse_event() via consume_name_changed(). 2. set_socket_connected() (2.05% CPU): added a lock-free early-return using load_relaxed(m_flags) to skip redundant lock acquisition when the socket is already in CONNECTED state. Supporting changes: - Add set_name_locked() for callers already holding exclusive_lock(). - Convert direct m_name assignments in parsers.cpp to use set_name()/set_name_locked() so the flag is set consistently. - Remove snapshot_oldname() call from get_fd() in threadinfo.h. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…nal project support
Introduce cmake/toolchains/zig.cmake, a generic Zig toolchain file that
derives CMAKE_SYSTEM_PROCESSOR from the ZIG_TARGET env var and locates
the zig executable via ZIG env var, CMAKE_C_COMPILER fallback, or PATH.
Refactor ExternalProjectToolchain.cmake to correctly propagate compiler
subcommands (CMAKE_*_COMPILER_ARG1), target flags, and multi-tool
archivers (via ENV{AR}/ENV{RANLIB}) to autotools-based external projects.
Add FALCOSECURITY_AUTOTOOLS_HOST_FLAG for cross-compilation --host support.
Update install-zig action to export ZIG, ZIG_TARGET, and
ZIG_TOOLCHAIN_FILE instead of wrapper scripts. Update CI to use
-DCMAKE_TOOLCHAIN_FILE.
Additional fixes for glibc 2.17 compatibility:
- Add gettid() fallback in libpman for glibc < 2.30
- Propagate toolchain CFLAGS into zlib's configure
- Use falcosecurity_external_project_cache_args in fmt
Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Concurrent remove_thread() calls from multiple workers cause corrupted internal state in folly::ConcurrentHashMap, leading to segfaults during shutdown. Add m_remove_thread_mutex and remove_thread_locked() to serialize all thread removal (parsers, sinsp::next, inactive-thread reaper) without holding the lock during the expensive per-thread teardown longer than necessary. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
m_lastevent_data, m_lastevent_cpuid, and m_lastevent_type on sinsp_threadinfo were shared across all workers, causing use-after-free races when two workers processed events for the same TID concurrently (e.g. due to CPU migration between syscalls). Move the enter-event storage into a per-parser (per-buffer) unordered_map keyed by TID. Since enter/exit events for a single syscall always land in the same ring buffer, per-buffer storage correctly pairs them without cross-worker contention. Remove the now-unused members and accessors from sinsp_threadinfo. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Move the zig.cmake toolchain file into the install-zig action directory and make cmake/toolchains/zig.cmake a symlink. The action now copies zig.cmake into ZIG_DIR at install time, removing the fragile dependency on the libs source tree path when the action is used from other repos. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Add a single-entry thread-local cache to threadinfo_map_impl_t (Folly path only), mirroring the existing fdtable TL cache pattern. This avoids repeated ConcurrentHashMap lookups for the same tid within a single event processing cycle. Profiling showed 702M thread lookups with 0% cache hit rate. The FD table's equivalent cache achieves ~14% hit rate, and thread lookups exhibit even higher temporal locality since consecutive syscalls from the same thread are common. The cache is invalidated on put/erase/clear. Stats are wired into find_thread() to distinguish cached vs non-cached lookups. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
The C preprocessor does not short-circuit: even when __GLIBC_PREREQ is undefined, the token __GLIBC_PREREQ(2, 30) on the same #if line causes a parse error. Split into nested #if/#endif so the function-like macro is only evaluated when it exists. Fixes the zig (musl) cross-compile. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
The m_last_lookup_cached member was a shared non-atomic field written by get_ref() from multiple worker threads, causing a TSAN data race. Move the flag into the thread_cache_entry struct which is already thread-local, and read it via was_last_lookup_cached() from the caller's own TL cache entry. This eliminates the race entirely. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
The previous fix (eba49985b) replaced the negated form with `defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 30)` but this still fails on musl/zig because the C preprocessor evaluates both operands of && in a #if directive — the function-like macro invocation __GLIBC_PREREQ(2, 30) is a parse error when the macro doesn't exist. Split into nested #if/#endif so __GLIBC_PREREQ(2, 30) is only reached when `defined(__GLIBC_PREREQ)` is true. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…ic guard Replace remove_thread_locked() and its global m_remove_thread_mutex with an atomic claim_removal() flag on each threadinfo, preventing double-removal races without serializing all removals. Make decrement_thread_count() return the remaining alive count to eliminate TOCTOU gaps when deciding whether to tear down the main thread. Also optimize copy_and_sanitize_path() with a fast-path that bulk-copies runs of normal printable ASCII, avoiding per-byte utf8_seq_len() calls. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
- event.cpp: correct sanitize_string calls (two-arg form with storage) and strcpy_sanitized calls (vector<char>& dest, string_view src) - strerror.h: restore accidentally removed scap_err_opnotsup and scap_err_unsupported_setting static inline helpers - folly-config.h: add _WIN32 guards to disable Linux-only features (weak symbols, accept4, pthread, VLA, ELF, etc.) on MSVC - fdinfo.cpp: fix sanitize_string call in tostring_clean() - threadinfo.cpp: fix get_path_for_dir_fd() — missing #endif, use dirfd_path instead of undeclared name, null-terminate readlink result Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Deep-copy the filter check list by calling allocate_new() on each sinsp_filter_check. Required by Falco's multi-thread worker context creation where each worker needs its own filter_check_list instance. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
In multi-worker mode, sinsp_threadinfo::m_lastevent_data was shared across workers processing events for the same TID, causing a heap-use-after-free when one worker freed the data while another read it. Move the entire lastevent state (data buffer, cpuid, type, fd) into a per-parser unordered_map<tid, per_tid_lastevent>. Each worker's parser now owns its own copy, keyed by TID. The fd/type fields are still written to tinfo via relaxed atomics for filter engine backward compat. Trade-off: if a thread migrates CPUs between enter and exit syscall, the exit may land on a different worker without stored enter data — retrieve_enter_event returns false (same graceful degradation as the existing "truncated at beginning of trace" path). Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…ame_changed atomic Replace all RETURN_EXTRACT_VAR / RETURN_EXTRACT_STRING macro calls with the upstream extract_single_val() / extract_single_string() methods and remove the macro definitions, aligning with the refactoring done in commits 5050dd4 and 8aa1a7f. Make sinsp_fdinfo_impl::m_name_changed a copyable_atomic_flag (thin wrapper around std::atomic<bool> that supports copy/move for defaulted constructors). This eliminates a TSAN data race where consume_name_changed() wrote the flag without a lock while clone() copied the object under a shared lock. The atomic exchange is lock-free with zero contention. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…w in render_fd fdinfo->get_name() returns a std::string by value (a temporary copy under shared lock). When passed directly to sanitize_string(), the temporary is implicitly converted to std::string_view, and sanitize_string() may return a view pointing into that temporary. After the temporary is destroyed at the semicolon, sanitized_name becomes a dangling string_view, causing use-after-scope when accessed in snprintf. Store the get_name() result in a named local variable so it outlives the sanitized_name view. Fixes the "Test sinsp-example and .scap files" CI failure (garbage fd names in diff) and the ASAN stack-use-after-scope in render_fd. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…er-return The atexit handler cleanup_resources() accesses g_inspector, which points to the stack-allocated sinsp in main(). Early return paths (table_mode, platform tests, insert_module failure) did not null g_inspector before returning, so the atexit handler would access the destroyed stack variable after main() returned. Add an RAII guard that nulls g_inspector when main() exits, regardless of the return path. This fixes the ASAN stack-use-after-return in sinsp::stop_capture(). Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Gate Folly and its dependencies (fmt, double-conversion, glog) on NOT WIN32 since Folly uses POSIX-only APIs (dlopen, pthread). Guard pthread.h and pthread_once in ppm_sc_names.c with #ifndef _WIN32, falling back to simple lazy init on Windows. Add gmtime_r/localtime_r shims for MSVC in utils.cpp. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
… mismatch The multi-thread build enables Folly ConcurrentHashMap which changes internal struct layouts. Since Folly headers cannot be exported via pkg-config, external consumers see mismatched ABI causing a segfault. Also export public compile definitions in pkg-config Cflags for correctness. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…sitives get_base_dir() was returning the dirfd path without a trailing slash, causing concatenate_paths() to produce malformed paths (e.g. "…/libsinsp_e2etest_tmpfile" instead of "…/libsinsp_e2e/test_tmpfile"). Restore the trailing slash that was lost during refactoring. Also guard set_name_locked() so m_name_changed is only set when the name actually differs, preventing false positives in fd.name_changed filterchecks. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
MSVC doesn't support GCC/Clang __atomic_* builtins. Add #ifdef _MSC_VER branches using volatile access for loads/stores and _Interlocked* intrinsics for RMW operations, with if-constexpr size dispatch for 32-bit and 64-bit types. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…ibs build Folly was unconditionally linked on Linux even without ENABLE_MULTI_THREAD, causing the shared-libs pkg-config test to fail: installed headers reference folly/concurrency/ConcurrentHashMap.h via LIBSINSP_USE_FOLLY but Folly headers are not installed alongside libsinsp. Gate both the Folly linkage and the LIBSINSP_USE_FOLLY definition on ENABLE_MULTI_THREAD, since ConcurrentHashMap is only needed for the concurrent sync policy. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Signed-off-by: irozzo-1A <iacopo@sysdig.com>
Two bugs cause the thread table to fill up and never recover: 1. has_recently_exited() uses a one-directional timestamp comparison (ts >= entry.ts) that always fails in the cross-buffer race it is meant to prevent: a parent's clone_exit (earlier timestamp) processed after the child's PROC_EXIT (later timestamp) was already handled by another worker. Fix by comparing absolute timestamp difference. 2. remove_inactive_threads() is gated behind is_default_buffer, but in multi-thread mode every worker uses a reserved (non-default) buffer handle, so periodic purging never runs. Fix by removing the gate and using compare_exchange_strong on m_last_flush_time_ns so exactly one worker wins per purging interval. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
…oting Add temporary INFO/WARNING/DEBUG logs to key thread lifecycle points: - has_recently_exited: log when a re-add is suppressed (with timestamps) - remove_inactive_threads: log table size before/after and purge counts (invalid vs stale breakdown) - parse_clone_exit: log when a child entry is being created and the child was not found in the table (potential orphan re-creation) - sinsp::next: warn when a PROC_EXIT verdict finds the thread already gone from the table (concurrent removal or ordering issue) This commit is intentionally separate so it can be easily reverted once the thread table leak is confirmed fixed. Signed-off-by: irozzo-1A <iacopo@sysdig.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Any specific area of the project related to this PR?
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The falco branch with multi-thread support can be found here.
To build assuming the two repositories are in the same directory, use the following:
Does this PR introduce a user-facing change?: