moonchen commented on issue #13358:
URL:
https://github.com/apache/trafficserver/issues/13358#issuecomment-5029807164
I think it's worth restating how ET_NET threading is supposed to work here,
because I believe these crashes are a symptom of that model being violated
rather than something to paper over at the queue level:
1. Each client connection and its transactions stay on one ET_NET thread.
2. The origin connections for those transactions run on that same ET_NET
thread.
3. This is maintained by scheduling continuations back onto that thread —
`adjust_thread` for the cache-open handoff, `migrateToCurrentThread` for origin
connections reused from the pool.
4. `nh->mutex` is a per-ET_NET-thread lock, and a stack can come to hold it
two ways:
- **Stacks originating in the NetHandler** hold it because the NetHandler
locks it directly — the net poll (`waitForActivity`) and the `InactivityCop`
(whose continuation mutex *is* `nh->mutex`) take it and hold it reentrantly
across a whole batch.
- **Stacks originating in the event loop** hold whatever mutex the
continuation carries — `process_event` locks `e->mutex == continuation->mutex`
before `handleEvent`. For the session/SM continuations in these stacks, that
carried mutex is the **session/vc** mutex, *not* `nh->mutex`, so they must
acquire `nh->mutex` **fresh** when they reach `add_to_*_queue`. (`HttpSM` never
locks it directly — it reaches it through `release → add_to_keep_alive_queue`.)
5. AIO completions are steered back to the ET_NET thread that spawned them —
reads via the AIO op's stored thread (`io.thread`, dispatched by
`op->thread->schedule_imm`), aggregated writes via the CacheVC's
`thread_affinity`, set in `new_CacheVC` to `cont->mutex->thread_holding`.
**What the crash stacks tell us — and what they don't.** All three abort in
a *fresh* `MUTEX_TRY_LOCK(nh->mutex)` inside `add_to_*_queue`, reached from a
continuation dispatched via `EThread::process_event` (the event loop, not the
poll). `Mutex_trylock` succeeds reentrantly when the current thread already
holds the lock, so a failure there tells us exactly one thing: **at that
instant, some thread other than the one running the crashing stack holds
`nh->mutex`.**
It does *not* tell us whether the crashing continuation is even on the right
thread. It can be either of these:
- **(a) the crashing stack is on the wrong thread** — the continuation for
an `nh->thread`-owned vc got scheduled elsewhere, tries `nh->mutex`, and fails
because the *owner* is legitimately holding its own lock in its poll. The lock
holder is behaving correctly; the bug is that the continuation isn't on its
thread.
- **(b) the crashing stack is on the owner, but a foreign thread holds the
lock** — something grabbed `nh->mutex` off-thread.
The follow-up report — the crash persisting after adding `if (this->thread
!= this_ethread()) return true;` to `add_to_active_queue` — is evidence that
(b) occurs at least for the H2 `create_stream` path (the guard short-circuits
(a), yet it still aborted). I wouldn't assume that for the HTTP/1.1 and
H2-teardown stacks; those could well be (a). Either way the root cause is a
continuation on the wrong thread, and the `add_to_*_queue` asserts are a
downstream tripwire for it — I'd rather find and fix the mis-scheduling than
weaken the assert.
**Finding the culprit.** Rather than reason further from the stacks, I think
the productive next step is to instrument this and let it tell us which path is
at fault. A few options, roughly in order of effort:
- **Rule out (a) from the existing cores.** At the aborting frame, compare
the vc's owner thread — `this->thread` (== `nh->thread`) — to the `EThread`
actually executing (the `this` of the `EThread::execute_regular` frame). If
they differ, it's (a), and the continuation was mis-scheduled.
- **Identify the holder for (b).** `nh->mutex->thread_holding` gives the
holding thread (present in release builds); a debug/lock-debug build also
records the acquiring file/line in `nh->mutex->srcloc`. Caveat:
`thread_holding` is cleared on release, so a *transient* holder can read back
as null in a post-mortem core (this is what @bryancall observed in #5950). If
the holder is the owner in its poll, it's a long hold and will still be
populated (pointing at `waitForActivity`).
- **Trace it live.** Finding *where* the wrong thread takes the lock means
catching acquisitions across all the lock sites, not just the queue functions —
the holder in (b) could be any path that touches `nh->mutex`. Two ways to do
that without a lot of ceremony:
- **`bpftrace`, no rebuild** (attractive since this reproduces in
production). The ProxyMutex lock helpers are inlined, so trace the underlying
`pthread_mutex_lock`/`pthread_mutex_trylock` — they see every acquisition
regardless of which variable named the mutex. Keep the "home thread" in a BPF
map keyed by the mutex address (seed it from a NetHandler symbol, e.g.
`waitForActivity`, so only NetHandler mutexes are tracked), and flag + capture
a stack whenever a non-home tid acquires one. It's heavy (those pthread calls
are hot process-wide), so use a short capture window and filter to tracked
addresses in-kernel.
- **A cheaper, probabilistic sample:** attach to the hot actor methods
(`UnixNetVConnection` / session / SM) and check `this->thread` vs
`this->nh->mutex->thread_holding` (or just the running tid vs `this->thread`).
That samples the "actor running off its owner thread" condition directly, at
much lower cost, without touching the lock primitives.
@wuxcer — if you can try one of these (the `bpftrace` route needs no
rebuild), the thread ids at the point of failure — the executing thread,
`vc->thread`, and `thread_holding` — plus a stack for the off-home acquirer
should tell us whether we're chasing (a) or (b), and which path put the
continuation on the wrong thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
