On 3/11/24, Pasha Tatashin <pasha.tatashin@xxxxxxxxxx> wrote: > On Mon, Mar 11, 2024 at 1:09 PM Mateusz Guzik <mjguzik@xxxxxxxxx> wrote: >> 1. what about faults when the thread holds a bunch of arbitrary locks >> or has preemption disabled? is the allocation lockless? > > Each thread has a stack with 4 pages. > Pre-allocated page: This page is always allocated and mapped at thread > creation. > Dynamic pages (3): These pages are mapped dynamically upon stack faults. > > A per-CPU data structure holds 3 dynamic pages for each CPU. These > pages are used to handle stack faults occurring when a running thread > faults (even within interrupt-disabled contexts). Typically, only one > page is needed, but in the rare case where the thread accesses beyond > that, we might use up to all three pages in a single fault. This > structure allows for atomic handling of stack faults, preventing > conflicts from other processes. Additionally, the thread's 16K-aligned > virtual address (VA) and guaranteed pre-allocated page means no page > table allocation is required during the fault. > > When a thread leaves the CPU in normal kernel mode, we check a flag to > see if it has experienced stack faults. If so, we charge the thread > for the new stack pages and refill the per-CPU data structure with any > missing pages. > So this also has to happen if the thread holds a bunch of arbitrary semaphores and goes off cpu with them? Anyhow, see below. >> 2. what happens if there is no memory from which to map extra pages in >> the first place? you may be in position where you can't go off cpu > > When the per-CPU data structure cannot be refilled, and a new thread > faults, we issue a message indicating a critical stack fault. This > triggers a system-wide panic similar to a guard page access violation > OOM handling is fundamentally what I was worried about. I'm confident this failure mode makes the feature unsuitable for general-purpose deployments. Now, I have no vote here, it may be this is perfectly fine as an optional feature, which it is in your patchset. However, if this is to go in, the option description definitely needs a big fat warning about possible panics if enabled. I fully agree something(tm) should be done about stacks and the current usage is a massive bummer. I wonder if things would be ok if they shrinked to just 12K? Perhaps that would provide big enough saving (of course smaller than the one you are getting now), while avoiding any of the above. All that said, it's not my call what do here. Thank you for the explanation. -- Mateusz Guzik <mjguzik gmail.com>
