feat(sinsp): CoW individual fdinfos - #3072
Open
gnosek wants to merge 7 commits into
Open
Conversation
poiana
added
kind/feature
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3072 +/- ##
==========================================
+ Coverage 76.43% 76.69% +0.26%
==========================================
Files 301 302 +1
Lines 33659 33968 +309
Branches 5054 5058 +4
==========================================
+ Hits 25726 26051 +325
+ Misses 7933 7917 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
|
/milestone 0.26.0 |
leogr
reviewed
Jul 30, 2026
leogr
left a comment
leogr
left a comment
Member
There was a problem hiding this comment.
Reviewed using https://github.com/leogr/falco-expert
One thing I believe we may want to address here: content_equals() is not virtual, while sinsp_fdinfo is an explicit subclassing point via build_fdinfo(). See my comments below 👇
Also, since this PR changes the plugin-visible behavior of get_table_entry() handles, I am not fully convinced that release-note: NONE is the right call.
Everything else is minor. Thanks 🙏
Two comments that were saying less than they should: upgrade_fd_info_writable() claimed that when the writable lookup comes up empty "callers cope with that the same way they cope with a null fd info". They don't -- they never see null, they get the pre-existing read-only pointer and dereference it. Spell out what a write through it would mean and what has to happen first for that to be reachable. The m_fdinfo_name_snapshot comment still carried the first line of the bool field it replaced, and did not say that snapshot validity is keyed on the fd pointer: replacing the event's fd mid-event also clears the name-change answer. Comments only, no functional change. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
Writable access paths (find_mut, loop, the plugin API's get_entry) now give the slot a private entry before handing out mutable access: if the entry may be referenced by another table, it is cloned in place. The copy decision is gated on a per-table hint, m_entries_maybe_shared, set when share_from() puts contents in play and cleared when a private generation is guaranteed (deep detach, clear, construction). The gate is what makes plain reference counts usable: on a never-shared table, extra references are plugin entry handles or caches, which must keep observing in-place writes -- copying there would orphan a handle someone is about to write through (the thread_manager.fdtable_access add_entry/get_entry pattern does exactly this). On a once-shared table the count check runs, disarming our own cache reference first; a stale cache elsewhere then costs nothing but the copy, while a live plugin handle is left pointing at the pre-copy entry, which is why get_entry() now spells out how long a handle is good for. find_mut also gains a cache fast path for the private-entry case, mirroring the const lookup cost on the hot per-event path. Inert for now: the detach is still deep, so cross-table entry references do not survive it. The shallow flip comes next. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
detach_if_shared() now copies only the map, sharing the entries with the previous contents; retain() on shared contents likewise keeps the surviving slots without cloning them. Individual entries are copied at writable handout (the machinery from the previous commit), so the cost of touching a table drops from "clone every fd" to "copy the map nodes" and each modified fd pays exactly one entry clone per table. Inherited fds that no process ever modifies now exist once, for their whole life, no matter how many processes hold them or how much unrelated fd activity those processes have -- this is where the memory win on fork-heavy workloads (hundreds of thousands of duplicated fdinfos) reaches its full size. The fork+execve pattern shares even the surviving non-CLOEXEC entries. Tests extended to pin the granularity: a write copies only the touched entry, add/retain/execve-purge leave untouched entries shared across tables. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
The debug dump at the end of a capture said how many fds the thread table holds in total, which after copy-on-write sharing is a count of fd slots, not of fd entries: an fd inherited by a thousand processes is one entry billed a thousand times. Report both, so the gap is the saving. Unique means distinct entry objects, not distinct contents maps. Sharing starts wholesale, but the first write anywhere in a table gives it a private map that still references the parent's entries, and per-entry sharing is the thing this series is about -- counting maps would stop seeing it exactly when it starts working. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
Full-state equality for fd entries: fd number, type, flags, identity (dev/mount/ino), names, and the union member selected by the type (the tuple unions compare their raw bytes, the server infos field by field to sidestep padding). Dynamic fields are not compared, so extensible_struct exposes has_dynamic_field_values() letting callers exclude entries that carry any. Virtual, like clone() and for the same reason: an event processor may build a subclass carrying extra data through build_fdinfo(), and two such entries agreeing on every base field are still not interchangeable. A predicate that could not see the extra data would answer true and let a caller share one entry where two are needed, with no way back once the first write clones the survivor. This is the equality the proc-scan deduplication pass (next commit) uses to decide that two scanned fds are the same inherited fd. Nothing about a wrong answer is loud -- it merges two distinct fds and lets one process observe the other's state -- so the tests walk every field of every fd type, change it alone, and demand that the answer flips. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
lookup_device() cleared m_mount_id whichever way the lookup went, so a failure was recorded as "resolved, to nothing": the entry kept m_dev == 0 and could never be asked again. The lookup fails whenever /proc/<tid>/mountinfo cannot be read, most obviously when the scanned process is already gone by the time one of its fds is first looked up. Clear the mount id only when it produced a device. Since libscap caches successful lookups globally in m_dev_list, keyed by mount id and independent of the tid, this costs a retry only for the entries that have no answer yet -- and a retry is exactly what they want: an fd's mount is listed in the mountinfo of every process that holds a file on it, so a table whose own tid is still readable can succeed where another one failed. Own commit because it is not new (9dc846f introduced the "don't try again" comment along with the behaviour) and it stands on its own. It is here because sharing changes what it costs: with entries shared between fd tables, one table giving up now takes the device away from every other table holding the same entry, where before it only ever cost the one process that looked first. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
…scan Fork-inherited fds appear once per process in /proc, so the initial scan materializes the same duplication the live fork path no longer creates: tens of thousands of processes can carry hundreds of thousands of copies of a few thousand distinct fds. After the scan (and after the socket direction fixups, which mutate entries), a single pass shares content-identical entries between tables, bucketed by the fork-inheritance signature (fd number, device, inode) and confirmed by full content equality. Adopting or donating a canonical entry flips the table's entries-maybe-shared hint, so copy-on-write isolates any later divergence exactly as it does for live forks; entries carrying dynamic field values are excluded (content_equals does not cover them), as are tables sharing contents wholesale (already deduplicated, and their maps must not be rebound in place). The pass runs in sinsp init for both live captures and scap files. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
leogr
approved these changes
Jul 31, 2026
Contributor
|
LGTM label has been added. DetailsGit tree hash: e501585b1a87792ea0e4a34cbda921af6b207f01 |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnosek, leogr 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 |
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
3 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.
This is the follow up to #3066, where we make individual fds copy on write as well.
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area libsinsp
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:
Does this PR introduce a user-facing change?: