Per-thread FD tables via unshare(CLONE_FILES) for ET_NET threads

[c-taylor]

Eliminate kernel spinlock contention on the shared files_struct by giving each ET_NET thread its own private FD table after all initialization FDs are in place. This removes the accept4/close contention visible as ~58% CPU in native_queued_spin_lock_slowpath at high thread counts.

This change is incompatible with MacOS and FreeBSD: The only equivalent solution there seems to be migrating to multi process. (This should be discussed as a long term goal)

Scheduled after start_HttpProxyServer() to ensure all persistent FDs (eventfds, cache disks, DNS sockets, log files, listen sockets, plugin FDs) are copied into each threads private table. Linux-only; non-fatal on failure (falls back to shared table).

I was also able to cause intermittent socket failures before applying this change.

[c-taylor]

I think this is ready to go, but would appreciate a confirmation from another source.

On 128thr systems I was able to move scaling from:
95,000 HS/sec (X25519), with socket errors
to
>400,000 HS/sec (X25519), no socket errors

Applying this change.

[c-taylor]

Heading off at least one expected code etiquette question:
There is no such equivalent in other OS, so using the distinct syscall names for functions inside the idef, for me was much clearer as intent.

[Copilot]

Pull request overview

This PR aims to reduce Linux kernel contention on the shared files_struct by giving each ET_NET thread its own FD table via unshare(CLONE_FILES) after server initialization, targeting accept/close spinlock hot paths at high thread counts.

Changes:

• Schedule unshare_et_net_fd_tables() after start_HttpProxyServer() in both the delayed-listen and non-delayed startup paths (Linux-only).
• Implement unshare_et_net_fd_tables() to schedule per-ET_NET thread unshare(CLONE_FILES) work on Linux.
• Expose the Linux-only unshare_et_net_fd_tables() declaration via P_UnixNet.h.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/traffic_server/traffic_server.cc	Calls unshare_et_net_fd_tables() after start_HttpProxyServer() (Linux-only).
src/iocore/net/UnixNet.cc	Adds Linux implementation that schedules unshare(CLONE_FILES) on each ET_NET thread.
src/iocore/net/P_UnixNet.h	Adds Linux-only prototype for unshare_et_net_fd_tables().

[c-taylor]

The concern here would be mutating FDs that change over lifetime of the process.
Arbitrary plugin usage perhaps being the largest risk.

Whilst this 'may' work in the general case, it might need some accompaniment of additional features to encourage the correct behaviour.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[c-taylor]

2 potential issues discovered:

1. diags output
2. ssl.keylog_file

[bneradt]

Looks fine. Too bad this doesn't work with global/hybrid. But maybe it is needed less there?

[JosiahWI]

@c-taylor Are you intentionally leaving this PR in draft for now?

[c-taylor]

Yes, until I have a chance to speak with someone about the two potential issues listed above.

I have not mitigated them in the PR as yet.

[bryancall]

Leaving this as a comment rather than a formal review since it is still a draft.

The measured win is large and real, 95,000 to 400,000+ X25519 handshakes per second on a 128-thread box with the socket errors gone, and several parts of the implementation are carefully done:

• The gate is narrow and correct on the axis it covers. exec_thread.listen == 1 is non-default, and the session-pool check correctly excludes global and hybrid, where migrateToCurrentThread() moves file descriptors by number.
• Both gating records are RECU_RESTART_TS, so no config reload can flip the process into an unsafe pool after unsharing.
• The #if defined(__linux__) is confined to the unshare call rather than the continuation, so the code compiles unchanged on OSX and FreeBSD.
• Event ordering against NetAccept is sound. ProtectedQueue::dequeue_external reverses the atomic list to preserve FIFO, so the unshare continuation is guaranteed to run before accept_per_thread on the same thread.
• Failure is non-fatal and falls back to the shared table rather than aborting startup, and the 20-line rationale comment above the continuation documents the mechanism and the migrateToCurrentThread hazard clearly.

The cross-thread file descriptor problem needs an answer before this can be merged

src/iocore/net/UnixNet.cc:241

You have noted this in the thread already, so I am not telling you anything new, but I want it written down concretely because it is the thing that decides whether the feature can ship as-is.

After unshare(CLONE_FILES), a file descriptor number is meaningful only in the table of the thread that created it. ATS keeps at least two process-wide descriptors that are minted on one thread and used from every ET_NET thread:

• TLSKeyLogger::_fd is a singleton static, opened at TLSKeyLogger.cc:65 and written with writev() at :102. enable_keylogging_internal() runs from SSLConfigParams::initialize() on the config reconfigure path, while TLSKeyLogger::log() runs from ssl_keylog_cb on ET_NET. The number is valid only in whichever table called open(), and that holds even if the open happens to land on an ET_NET thread.
• diags_log->m_fp. DiagsLogContinuation is scheduled on ET_TASK, and should_roll_diagslog() (Diags.cc:538-600) fopen()s a fresh BaseLogFile and swaps the process-wide diags_log under a lock. Every ET_NET Note, Warning, Error and Dbg then writes to that descriptor number in its own private table.

Both are behind non-default records, so this is not a default-config corruption bug. Both are supported operator-facing features, though, and the failure mode is silent: writes land in whatever the number happens to mean in that thread, which in the keylog case means TLS session secrets going to an unrelated socket. Nothing bounds which descriptor gets hit and nothing detects it.

I do not think this needs to block the idea, only the current gating. The options I see are narrowing the gate further to also require that keylogging and diags rolling are off, or introducing a dedicated opt-in record rather than overloading exec_thread.listen, or making those two descriptors thread-aware. Worth deciding before more work goes into the current shape.

No coverage at all

src/iocore/net/UnixNet.cc:231-253

grep over tests/ finds proxy.config.exec_thread.listen exactly once, in tests/gold_tests/records/legacy_config/full_records.config set to 0, which is a records-parsing fixture rather than a running-server config. No autest starts ATS with the setting on, so listen_per_thread is 0 in every CI run and the body of the if never executes anywhere. The 15 green checks are consistent with this code being arbitrarily broken.

Partly mitigating: exec_thread.listen has no runtime autest coverage on master either, so the gap is not wholly introduced here. It matters more now, because the change alters kernel file-descriptor semantics for every ET_NET thread. A minimal autest that starts with exec_thread.listen=1 and server_session_sharing.pool=thread, drives traffic, then forces a diags-log roll would exercise the interesting path, and would also reproduce the problem above.

Smaller items

• src/iocore/net/UnixNet.cc:242 unshare() failure is reported only through Dbg. Under Docker's default seccomp profile the call returns EPERM on every thread, all threads keep the shared table, and the operator who enabled this specifically for the optimization sees nothing at all unless they already know to turn on the broad iocore_net tag. A performance feature that silently does not engage is worse than one that refuses loudly. Log once, with an atomic flag or on thread 0, rather than suppressing entirely. The pool-skip branch at line 240 has the same problem.
• src/iocore/net/UnixNet.cc:238-239 The pool gate fails open on a missing record: if RecGetRecordStringAlloc returns nullopt, pool && *pool != "thread" is false and the code falls through to unshare, which is the unsafe direction. It also re-parses the raw record string instead of reading the already-parsed server_session_sharing_pool enum from HttpConfig. The default is thread so the practical impact today is nil, but a safety gate should deny on unknown input.
• src/iocore/net/UnixNet.cc:236-238 Both record lookups run on every ET_NET thread. On a 128-thread box that is 256 record-table lock acquisitions for a value that is identical everywhere and cannot change. Reading them once in exec_thr_late_init() and returning early would also avoid scheduling 128 no-op continuations, and would give the logging fix a single place to live.
• src/iocore/net/P_UnixNet.h:42 exec_thr_late_init() is an undocumented unnamespaced global whose name conveys nothing, declared in a net-private header and called from traffic_server.cc. The ordering requirement is load-bearing and lives only in a source comment at the call site. Something like net_unshare_et_net_fd_tables() plus a header comment restating the ordering constraint would carry the invariant to the person who needs it.
• src/iocore/net/UnixNet.cc:209-212 The header comment lists plugin file descriptors among those guaranteed to be in place before the unshare. Descriptors opened from TS_LIFECYCLE_PORTS_READY_HOOK, which fires at HttpProxyServerMain.cc:373 inside start_HttpProxyServer and therefore after this runs, are not covered, and neither is anything a plugin opens at runtime. You acknowledged the arbitrary-plugin risk in an earlier comment; the comment currently overstates the guarantee. Also worth a dedicated debug tag rather than reusing the broad iocore_net one.

One thing I chased and am withdrawing: I initially thought the schedule_imm with no barrier let main-thread descriptors created during start_HttpProxyServer land in some ET_NET tables and not others. Looking at it properly, the ordering holds, so that is not a live problem.