Hmm, another approach would be to use smp_rendezvous() with the CPU the thread is running on to capture the stack. This wouldn't introduce the weirdness with having to drop locks, etc. and avoid the race handling for the tid changing, etc. (It also wouldn't block but would grab the current stack frame instead of a stack frame at some point in the future.)

The in_panic change looks a bit interesting, but I'm curious if that is actually used. A userland process can't be scheduled to run to even invoke this sysctl if the system is panic'ing. Does Isilon use stack_save_thread() in other places?

Sorry, maybe I didn't investigate this enough, but I have to ask some questions:

Specified timeout is extremely short and the code assumes that ctx is removed from stack_hardclock_list (just zeroes it and retries). This seems risky.

In Linux there is a 'softlockup detector'. Once a cpu appears to be stuck there is an option to send NMIs to all cpus to collect all traces. Usefulness of such detector aside, such mechanism would allow people to collect traces even if given cpu is stuck with interrupt disabled. It also avoids the branch in hardclock and the problem with making sure stuff is no longer on the list.

In D323#5, @jhb wrote:

The in_panic change looks a bit interesting, but I'm curious if that is actually used. A userland process can't be scheduled to run to even invoke this sysctl if the system is panic'ing. Does Isilon use stack_save_thread() in other places?

Yeah, it is or was used at Isilon by some kernel component to gather stacks during panic (without having done any research as to what component, sorry). It's not useful for the ^T backtrace functionality. We could drop that piece for this review.

In D323#4632, @jhb wrote:

Hmm, another approach would be to use smp_rendezvous() with the CPU the thread is running on to capture the stack. This wouldn't introduce the weirdness with having to drop locks, etc. and avoid the race handling for the tid changing, etc. (It also wouldn't block but would grab the current stack frame instead of a stack frame at some point in the future.)

In principle, I agree with this proposal, it should be much more reliable and simpler than what is done in the patch.

The issue I see is that stack walker would need to walk over the interrupt frames, there is no support in stack_save() to detect and step over them. Still, the traces above seems to successfully work with the interrupts.

The in_panic change looks a bit interesting, but I'm curious if that is actually used. A userland process can't be scheduled to run to even invoke this sysctl if the system is panic'ing. Does Isilon use stack_save_thread() in other places?

@markj has been working on converting this to use smp_rendezvous and cleaning up some other issues with it.

In D323#62895, @bdrewery wrote:

@markj has been working on converting this to use smp_rendezvous and cleaning up some other issues with it.

Yup, sorry, just been busy. I'll put it up soon.

One annoyance is that smp_rendezvous() can't be called with the target thread's lock held, else we may deadlock if the thread is spinning with interrupts disabled on its own thread lock. So the IPI handler needs to handle the case where the thread was migrated to a different CPU after we dropped the thread lock but before the interrupt was handled. I've fixed this by just detecting this case and retrying, but that doesn't seem ideal - any suggestions?

I thought about adding a new thread inhibitor for stack tracing, but that might be overkill.

In D323#63297, @markj wrote:

One annoyance is that smp_rendezvous() can't be called with the target thread's lock held, else we may deadlock if the thread is spinning with interrupts disabled on its own thread lock. So the IPI handler needs to handle the case where the thread was migrated to a different CPU after we dropped the thread lock but before the interrupt was handled. I've fixed this by just detecting this case and retrying, but that doesn't seem ideal - any suggestions?

thread_lock_flags_() reenables interrupts periodically while spinning, same as the normal spinlock. But indeed counters are set up such that the lock failure might be declared.

Did you considered checking for the stack trace request in thread_lock_flags() while spinning, and directly satisfying it from the locking function ?

In D323#63323, @kib wrote:

In D323#63297, @markj wrote:

One annoyance is that smp_rendezvous() can't be called with the target thread's lock held, else we may deadlock if the thread is spinning with interrupts disabled on its own thread lock. So the IPI handler needs to handle the case where the thread was migrated to a different CPU after we dropped the thread lock but before the interrupt was handled. I've fixed this by just detecting this case and retrying, but that doesn't seem ideal - any suggestions?

thread_lock_flags_() reenables interrupts periodically while spinning, same as the normal spinlock. But indeed counters are set up such that the lock failure might be declared.

If the thread already holds a different spinlock, thread_lock_flags_() will not reenable interrupts.

Did you considered checking for the stack trace request in thread_lock_flags() while spinning, and directly satisfying it from the locking function ?

I hadn't, but that could work. I'm not sure where the thread would store the stack in this case though. Perhaps have a TDF_STACK and a struct stack *td_stack in the struct thread. If one wishes to capture the stack of a running thread, they:

• set td->td_stack = st, and set TDF_STACK in the target thread
• drop the thread lock and call smp_rendezvous_cpus() to interrupt the target thread
• if the target thread was interrupted, the handler acquires its thread lock, clears TDF_STACK, and captures the stack
• otherwise the thread must have changed states, so it must have acquired its own thread lock at some point(?), at which point it stores its stack in td_stack and clears TDF_STACK. The rendezvous handler will see this and can just return.

Does that seem reasonable? Adding a new thread flag and member, and modifying thread_lock() just for this case seems like a somewhat intrusive solution to me, compared to adding a new thread inhibitor.

In D323#63341, @markj wrote:

If the thread already holds a different spinlock, thread_lock_flags_() will not reenable interrupts.

Which can be reasonably interpreted as the flaw in the approach. I start thinking that what should be done is to broadcast (?) NMI with some global variables pointing to thread and to struct stack to fill. NMI handler should detect the situation and fill the stack trace if current thread is equal to the stored pointer.

Did you considered checking for the stack trace request in thread_lock_flags() while spinning, and directly satisfying it from the locking function ?

I hadn't, but that could work. I'm not sure where the thread would store the stack in this case though. Perhaps have a TDF_STACK and a struct stack *td_stack in the struct thread. If one wishes to capture the stack of a running thread, they:

• set td->td_stack = st, and set TDF_STACK in the target thread
• drop the thread lock and call smp_rendezvous_cpus() to interrupt the target thread
• if the target thread was interrupted, the handler acquires its thread lock, clears TDF_STACK, and captures the stack
• otherwise the thread must have changed states, so it must have acquired its own thread lock at some point(?), at which point it stores its stack in td_stack and clears TDF_STACK. The rendezvous handler will see this and can just return.

Does that seem reasonable? Adding a new thread flag and member, and modifying thread_lock() just for this case seems like a somewhat intrusive solution to me, compared to adding a new thread inhibitor.

For this, I believe td_stack would be enough. Clear td_stack if the trace was filled. It would give the same effect as the td_flags flag. This is only IMHO.

In D323#63477, @kib wrote:

I start thinking that what should be done is to broadcast (?) NMI with some global variables pointing to thread and to struct stack to fill. NMI handler should detect the situation and fill the stack trace if current thread is equal to the stored pointer.

That seems like a morally correct approach if we think of this as an attempt to profile the target thread. I think a broadcast NMI could be avoided if the target thread lock is held throughout the operation, since we could then deliver an NMI to td_oncpu and expect the target thread to be running (perhaps spinning on its thread lock). There should probably also be some check to ensure that we are allowed to interrupt the target thread, perhaps with p_candebug().

The downside is that more MD code is required, and I'm not yet sure whether this can be done on all architectures. But, it should be doable on x86, with stubs for other architectures. I'll give this a try.

Did you considered checking for the stack trace request in thread_lock_flags() while spinning, and directly satisfying it from the locking function ?

...
Adding a new thread flag and member, and modifying thread_lock() just for this case seems like a somewhat intrusive solution to me, compared to adding a new thread inhibitor.

For this, I believe td_stack would be enough. Clear td_stack if the trace was filled. It would give the same effect as the td_flags flag. This is only IMHO.

Ah, I think you're right.

In case anyone's curious:

I implemented a stack-saver for amd64 using NMIs and modified kern.proc.kstack here: https://people.freebsd.org/~markj/patches/stack_nmi.diff

I then reimplemented stack-on-siginfo: https://people.freebsd.org/~markj/patches/siginfo_stack.diff

These appear to work, modulo some small bugs. But the second patch isn't correct: it resolves kernel symbols in tty_info(), which requires taking a sleepable lock with a tty mutex held. In Isilon's OS this is handled by resolving the symbols and printing to the tty in a taskqueue handler. But that seems excessively complicated: I think we should be able to resolve the stack without potentially sleeping (and I've hit this limitation before), so I'd like to try extending the linker KPI to accommodate this.

In D323#64235, @markj wrote:

In case anyone's curious:

I implemented a stack-saver for amd64 using NMIs and modified kern.proc.kstack here: https://people.freebsd.org/~markj/patches/stack_nmi.diff

This looks simple and clean enough.

Suppose that nmi_stack was set by CPU0 and nmi sent to CPU1, Assume that CPU2 got an NMI for any other reason meantime. Wouldn't the assert curthread == nmi_pending fire on CPU2 ? I do not think that the argument that the stack_save_td_running() locks nmi_lock is enough, e.g. NMI could come from some hardware.

Another thing which I cannot resist to point out is that the NULL write to nmi_pending must have release semantic, i.e. atomic_store_rel_ptr(&nmi_pending, NULL), and corresponding loop in stack_save_td_running() must use acquire load. This way, the stack trace writes in nmi handler are guaranteed to be visible in the calling cpu. I understand that the amd64 strictness makes this purely academic, but I believe that acq/rel use would highlight the intent and avoid the issue for relaxed arches when the code will be copy/pasted.