kernel: nothread: use k_timer to sleep instead of busy waiting

[lemrey]

The current implementation of k_sleep(), when multi-threading is disabled, busy waits using k_busy_wait() until the sleep timeout has expired.

This patch aims to improve power efficiency of k_sleep() for single-threaded applications by starting a timer (k_timer) and idling the CPU until the timer interrupt wakes it up, thus avoiding busy-looping.

[bjarki-andreasen]

I found this function k_timer_status_get() which can be used instead of the timer expired handler and atomic, returns positive if timer has elapsed, just pass NULL, NULL to k_timer_start

[bjarki-andreasen]

use atomic_t for atomics

[lemrey]

I can use atomic_t, but this is not an "atomic" variable; there is no race to access this variable.

[bjarki-andreasen]

use struct k_spinlock for spinlocks (critical sections)

[bjarki-andreasen]

also define variables at the top of the function (MISRA)

[lemrey]

Regarding the use of a spinlock instead of irq_lock(), I think I understand why it would be preferrable in general, but in this specific case when multithreading is disabled, SMP out of the picture. With that assumption, irq_lock() achieves the same as a spinlock, while also providing me the key to pass to k_cpu_atomic_idle(). I can't use k_cpu_atomic_idle() with a spinlock key directly.

[JarmouniA]

irq_lock() achieves the same as a spinlock

irq_lock() is a legacy API and we want to avoid introducing new usage of it.

[bjarki-andreasen]

static private variables should not use a namespace which could clash with a public definition, just use sleep_timedout

[lemrey]

Ok, I will rename

[lemrey]

I can update this to use k_timer_status_get(), but could you clarify why are you suggesting that? Is it so that we avoid declaring one function and one variable, or are there other reasons?

[lemrey]

Regarding the use of a spinlock instead of irq_lock(), I think I understand why it would be preferrable in general, but in this specific case when multithreading is disabled, SMP out of the picture. With that assumption, irq_lock() achieves the same as a spinlock, while also providing me the key to pass to k_cpu_atomic_idle(). I can't use k_cpu_atomic_idle() with a spinlock key directly.

[lemrey]

Ok, I will rename

[lemrey]

I can use atomic_t, but this is not an "atomic" variable; there is no race to access this variable.

[peter-mitsis]

I think this is OK.

[bjarki-andreasen]

I believe we should use the existing API for waiting on a timer:

static K_TIMER_DEFINE(sleep_timer, NULL, NULL);

/* This is a fallback implementation of k_sleep() for when multi-threading is
 * disabled. The main implementation is in sched.c.
 */
int32_t z_impl_k_sleep(k_timeout_t timeout)
{
	__ASSERT(!arch_is_in_isr(), "");

	SYS_PORT_TRACING_FUNC_ENTER(k_thread, sleep, timeout);

	k_timer_start(&sleep_timer, timeout, K_FOREVER);
	k_timer_status_sync(&sleep_timer);

	SYS_PORT_TRACING_FUNC_EXIT(k_thread, sleep, timeout, 0);

	return 0;
}

and update the implementation here:

zephyr/kernel/timer.c

Lines 270 to 285 in af171d4

	do {
	k_spinlock_key_t key = k_spin_lock(&lock);
	if (!z_is_inactive_timeout(&timer->timeout)) {
	result = *(volatile uint32_t *)&timer->status;
	timer->status = 0U;
	k_spin_unlock(&lock, key);
	if (result > 0) {
	break;
	}
	} else {
	result = timer->status;
	k_spin_unlock(&lock, key);
	break;
	}
	} while (true);

to something like:

		do {
			unsigned int key = irq_lock();

			if (!z_is_inactive_timeout(&timer->timeout)) {
				result = *(volatile uint32_t *)&timer->status;
				timer->status = 0U;
				if (result > 0) {
					irq_unlock(key);
					break;
				}
				k_cpu_atomic_idle(key);
			} else {
				result = timer->status;
				irq_unlock(key);
				break;
			}
		} while (true);

this way we have one implementation for waiting on a timer which works with and without threads, instead of one implementation for k_sleep, and another for k_timer_get_status_sync, both of which work without threads

[lemrey]

@bjarki-andreasen ok I'll try and do that change to timer.c in a different PR, let's see if that's accepted.

[lemrey]

Here #101758

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[lemrey]

@bjarki-andreasen please have another look

[bjarki-andreasen]

That is pretty cool