k_thread_foreach api breaks real time semantics

[mubes]

Describe the bug
When 'kernel stacks' or 'kernel threads' commands are entered at the command prompt realtime performance is lost.

To Reproduce

1. Enable shell, kernel extensions and thread/stack info. Specifically, the following must be defined;
   CONFIG_INIT_STACKS, CONFIG_THREAD_MONITOR, CONFIG_THREAD_STACK_INFO
2. Setup short period secondary task which is dependent on external interrupt (e.g. to toggle a pin in response to incoming pin change)
3. Compile (tested on STM32F429 build)
4. At the command prompt type 'kernel stacks' or 'kernel threads'
5. Observe that realtime response of other task is lost.

I think this is due to the k_spin_lock in kernel/thread.c::k_thread_foreach ... would it not be sufficient to just do a k_sched_lock here? Worst consequence would be incorrectly reported threads/stacks for the case that the printing task manipulated one of the, which it's not supposed to do anyway.

Expected behavior
Interrupt responding second task should continue to operate correctly.

Impact
What impact does this issue have on your progress (e.g., annoyance, showstopper)
Annoyance. Prevents use of kernel threads/kernel stacks commands on a realtime system.

Screenshots or console output
N/A

Environment (please complete the following information):
Linux, Zephyr SDC, 1.3.99, a4e950a

[andrewboie]

Is this really medium priority? The shell is a debugging feature..

[nordic-krch]

Issue is about k_thread_foreach which is used by the shell command.

zephyr/kernel/thread.c

Line 59 in 95e8a58

key = k_spin_lock(&lock);

@andrewboie @andyross can you look at it? If after this functions is called realtime response is lost then this is something very wrong.

[andyross]

Yeah, taking a lock around an iteration is bad practice for sure.

The only thing that lock is protecting is the threads list itself, there's absolutely no reason for it to be held while the callback is invoked. Except that the list iteration is hand-coded and probably isn't safe vs. removal during iteration.

Fixing this correctly should:

1. Use an slist and not a hand-coded list for ?@#!! sake
2. Iterate with SYS_SLIST_FOR_EACH_NODE_SAFE()
3. Lock with a spinlock only around the list operations (i.e. one big lock around the loop, and unlock/relock around the callback)

Alternatively:

• Check that no current callbacks passed to k_thread_foreach() attempt to create or destroy threads, and if they don't, document that as a limitation and simply remove the lock entirely.

Unfortunately replacing the _kernel.threads list with an slist will break OpenOCD compatibility. Both _kernel.threads and the next_thread member used to link them are exported (see subsys/debug/openocd.c).