documentation says giving a semaphore can release IRQ lock

[pabigot]

The documentation for preventing interrupts includes:

If thread A locks out interrupts then performs an operation that allows thread B to run (e.g. giving a semaphore or sleeping for N milliseconds), the thread’s IRQ lock no longer applies once thread A is swapped out.

This is wrong, no? Giving a semaphore when interrupts are masked is not supposed to release the interrupt mask, even if doing so makes a higher priority thread ready. But I'm not sure that's true if it's being given from the context of a preemptible thread.

If the same behavior applies to spin-locks on uniprocessors I think that's a big problem.

[pabigot]

@andrewboie @andyross can this documented-but-not-expected release of a lock actually happen as described?

[pabigot]

The more I think about it the more likely it seems this is the intended behavior, so I've reduced this to a question.

[andrewboie]

I think the documentation is wrong.

There are 3 entry points for rescheduling:

static inline void z_reschedule_unlocked(void)
{
	(void) z_reschedule_irqlock(arch_irq_lock());
}

void z_reschedule(struct k_spinlock *lock, k_spinlock_key_t key)
{
	if (resched(key.key) && need_swap()) {
		z_swap(lock, key);
	} else {
		k_spin_unlock(lock, key);
	}
}

void z_reschedule_irqlock(uint32_t key)
{
	if (resched(key)) {
		z_swap_irqlock(key);
	} else {
		irq_unlock(key);
	}
}

For the spinlock case, it's expected that the particular lock passed in is released at context switch, this is by design. Otherwise, if interrupts were disabled on the current CPU (because irqs were locked, or we were holding some other spinlock), the check in resched() will fail:

static inline int resched(uint32_t key)
{
#ifdef CONFIG_SMP
	_current_cpu->swap_ok = 0;
#endif

	return arch_irq_unlocked(key) && !arch_is_in_isr();
}

So I believe we're fine in the code and the docs are wrong.

[pabigot]

@andrewboie I'm concerned about cases like:

k_spinlock_key_t key = k_spin_lock(&my_lock);

// stuff
k_queue_append(&the_queue, &data);
// more stuff
k_spin_unlock(&my_lock, key);

AFAICT (haven't tested) if a higher priority thread is waiting on the_queue it will be given control before // more stuff which may not be what the author would have expected. The documentation says this happens for irq_lock and I believe it.

For this case the k_sem_give() needs to be moved outside the spinlocked region. I knew this at one point, and it's why "reschedule point" needs to be identified as part of the side effects for a lot of kernel API so that API that could have this affect isn't invoked while an external lock is held, but it is a little subtle.

[andrewboie]

I tried to explain this, but let me re-state in terms of the example you provided. The k_queue_append() call in your example should not trigger a context switch, because interrupts were already disabled on the local CPU when it was called.

The documentation says this happens for irq_lock and I believe it.

I just said this isn't true based on code inspection...

AFAICT (haven't tested)

Please have some supporting data if we're going to raise alarm bells about this.

[pabigot]

@andrewboie Thanks; I've done some testing and proposed a rewording based on what I observe. It may still be wrong in some nuanced way, but at least it doesn't incorrectly claim that giving a semaphore can steal an irqlock.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[pabigot]

This is the last time I'm removing stale from this; #29800 needs to get some attention.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[pabigot]

Consensus was the documentation is wrong, so one last attempt to get this addressed.