Reimplement stack capture of running threads.

[markj]

Summary

Change stack_save_td() to support stack capture for running threads, and
introduce a return value.

Reimplement stack capture for running threads on x86 using
smp_rendezvous instead of an NMI. It has become too difficult to deal
with all of the possible scheduler states, and we cannot capture a stack
while interrupts are disabled anyway.

[kib]

Isn't the code to capture stack trace become MI ?

sys/amd64/amd64/trap.c

55 ↗

(On Diff #67266)

I this line still needed ?

sys/x86/x86/stack_machdep.c

109 ↗

(On Diff #67266)

I think this would leave out almost all threads. The issue is that we set up td_frame e.g. on syscall entry and keep it there. Fast interrupts and some MD handlers e.g. local timer when calling into MI code, set up td_intr_frame but do not touch td_frame. Also td_frame could be invalid or NULL for kernel threads that never trap from userspace.

[markj]

sys/x86/x86/stack_machdep.c

109 ↗

(On Diff #67266)

I meant to write td_intr_frame, but even that is not enough since we do not set it for rendezvous interrupts.

One solution would be to re-add IPI_TRACE and handle it from ipi_bitmap_handler(), but then the code would no longer be MI. Another possibility is to modify MD callers of smp_rendezvous_action() to pass a trapframe, and have smp_rendezvous_action() use it to set td_intr_frame. Do you see any problems with the second approach?

[kib]

sys/x86/x86/stack_machdep.c

109 ↗

(On Diff #67266)

I think #2 would work, but I somewhat irrationally do not like it.

That said, why do you need to check for usermode interruption ? It is displaying something like <empty> or <usermode> vs. one- or two-frames stack.

[markj]

sys/x86/x86/stack_machdep.c

109 ↗

(On Diff #67266)

It is not really necessary to check, since we validate that the frame pointer lies within the kernel stack. But, we still need a way to get the interrupted thread's frame pointer. The use of td_pcb below is also wrong for running threads.

The validation of the FP is MD and I do not think that all implementations of stack_capture() check this. So I suspect now that it is preferable to keep the implementation x86-specific and use a new IPI vector.

[kib]

sys/x86/x86/stack_machdep.c

109 ↗

(On Diff #67266)

Well, ok.

[mjg]

sys/x86/x86/stack_machdep.c

143 ↗

(On Diff #67266)

Is there anything apart from thread lock being held here which causes the stack_save_td caller to run with interrupts disabled?

I think there are 2 issues here:

1. callers should be required to not have interrutps disabled, perhaps modulo except for thread lock. then there is no need to check spinlock_count in the first place.
2. since thread lock is dropped nothing prevents the target thread from exiting and getting reallocated. I don't see a good way to either prevent it or verify it did not happen. The least which can be done is validating in the dumping handler that the thread belongs to the expected process (by that i mean both the pointer and process start time match -- see microuptime(&p2->p_stats->p_start) in fork).

[mjg]

sys/x86/x86/stack_machdep.c

143 ↗

(On Diff #67266)

I just realized the caller is already required to not hold any other spinlocks since smp_rendezvous_cpu must be called with interrupts enabled. Therefore the spinlock_count check can be just removed as it is.

[mjg]

sys/x86/x86/stack_machdep.c

124 ↗

(On Diff #67266)

Is this really assertable? can the thread become swapped after the lock is dropped below? looks like this should be just checked

[markj]

sys/x86/x86/stack_machdep.c

124 ↗

(On Diff #67266)

As noted below, the caller prevents this either by holding the proc lock, or by ensuring that the thread is not already running, in which case the thread lock ensures that it cannot transition to TDS_SWAPPED.

143 ↗

(On Diff #67266)

I made a lot of mistakes in this diff. You are right that we do not need to check spinlock_count. I meant to check curthread's spinlock count, not td's, but it is not needed anyway. In cases where the thread may be running, we hold the process lock, which prevents both swapping and exiting. I will add an assertion for this.

[markj]

• Switch to using ipi_cpu(). This requires more synchronization that was previously provided by smp_rendezvous.
• Distinguish between empty stacks (e.g., for interrupt threads that are in the scheduler but have never executed) and threads running in user mode. I think this distinction is useful in procstat output.

[kib]

sys/x86/x86/stack_machdep.c

103 ↗

(On Diff #67422)

I think you do not need intr_success, it is enough to check that the stack was indeed captured in stack_save_td().

134 ↗

(On Diff #67422)

Wouldn't it better to gather all asserts at the start of the function, so that the consumer not miss anything in casual testing ?

143 ↗

(On Diff #67422)

Don't you need to clear intr_pending before ipi ? Also I would suggest to prefix all static vars in this file with e.g. stack_, since they are visible in debugger.

[markj]

Apply kib's suggestions.

[bdragon]

This removes the public kernel function stack_save() from some of the platforms.

I don't think this is intended.

[markj]

Remove just stack_save_td_running(), not everything between it and EOF.

[jhibbits]

sys/powerpc/powerpc/stack_machdep.c

111 ↗

(On Diff #67441)

Don't you want to remove stack_save_td_running() and keep stack_save()?