doc: kernel: interrupts: correct description of irq lock behavior #29800
Conversation
Contrary to the documentation giving a semaphore while an IRQ lock is held does not release the lock and give control to another thread. The release-lock behavior is observed only if the lock-holding thread sleeps. However the opportunity to reschedule will have been lost so it may be necessary to explicitly yield to allow the higher-priority thread to be serviced. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
| thread B has also locked out interrupts using its own IRQ lock. | ||
| (Whether interrupts can be processed while the kernel is switching | ||
| between two threads that are using the IRQ lock is architecture-specific.) | ||
| then performs an operation that puts itself to sleep (e.g. sleeping |
There was a problem hiding this comment.
Hmm I think we should document sleeping while interrupts are disabled or a spinlock held as undefined behavior or just not allowed. Is there anything that currently relies on this?
Seems a little crazy to expect a sleep to work in this situation. Linux absolutely forbids this, although on Linux spinlocks are implemented a little differently and have APIs which on uniprocessor distinguish between simply disabling preemption, and disabling preemption + disabling IRQs as well. But sleeping is not allowed regardless of whether you got the spinlock via spin_lock() or spin_lock_irqsave(), and it's not like Zephyr is incorrect; unconditionally disabling interrupts for all spinlocks is correct it's just not quite as efficient (and could be added in the future). Further reading: https://www.kernel.org/doc/Documentation/locking/spinlocks.txt
@andyross what do you think about this? I think this should just be UB. IOW, add another assertion to k_sleep() that interrupts are currently unlocked, not just check arch_is_in_isr() like it currently does.
|
|
||
| When thread A eventually becomes the current thread once again, the kernel | ||
| re-establishes thread A's IRQ lock. This ensures thread A won't be | ||
| interrupted until it has explicitly unlocked its IRQ lock. | ||
|
|
||
| If thread A does not sleep but does make a higher-priority thread B |
There was a problem hiding this comment.
This is a good point. I'm not sure if our documentation is clear enough on this point in other relevant places; nobody should assume that the scheduling gets pended and then acted upon as soon as interrupts or the spinlock is released.
Seems like in most cases k_yield() is the right API to invoke the scheduler after re-enabling interrupts or releasing the lock...but I think co-op threads may not want this...Maybe for code where it's not known if it will be used in preempt or co-op context we need something like k_yield_if_not_coop_thread() or something like that? (which would just call z_reschedule_unlocked())
There was a problem hiding this comment.
Looks like this can be handled with:
if (k_is_preempt_thread() != 0) {
k_yield();
}
without creating new API.
There was a problem hiding this comment.
The phrasing here seems fine. What I think the older documentation is trying to express is that it used to be the case that threads could be suspended while they held a lock. IRQ locks are recursive, you can take them multiple times and it's not possible for the scheduler to know what the onion looks like up the call stack.
Spinlocks aren't like that and will deadlock (or assert on uniprocessor if you have CONFIG_SPIN_VALIDATE) if you try to use a single one recursively. It's no longer legal to sleep while holding a lock, and code doesn't do that. (Actually now that I look I don't see the assert that used to be there to catch the case where you pend _current with a key value that won't unmask interrupts, I need to add that back.)
Contrary to the documentation giving a semaphore while an IRQ lock is held does not release the lock and give control to another thread. The release-lock behavior is observed only if the lock-holding thread sleeps.
However the opportunity to reschedule will have been lost so it may be necessary to explicitly yield to allow the higher-priority thread to be serviced.
Closes #29610