Skip to content

Conversation

@KushnerovMikhail
Copy link

@KushnerovMikhail KushnerovMikhail commented Nov 6, 2024

Fix k_sleep implementation for no multi-threading mode.
Fixes #81210

Absolute value of timeout expiration was fed to the k_busy_wait() function instead of delta value. That caused bug like incrementing of sleep time in geometric progression (while actual function argument is constant) during program running.

You can check it by simple program with CONFIG_MULTITHREADING=n option:

#include <zephyr/kernel.h>

int main (void)
{
	uint32_t new_ticks = 0;
	uint32_t old_ticks = sys_clock_tick_get_32();

	while (1) {
		k_sleep(K_MSEC(1000));
		new_ticks = sys_clock_tick_get_32();
		printf("%u\n", new_ticks - old_ticks);
		old_ticks = new_ticks;
	}

	return 0;
}

Output from qemu_riscv64
Before fix:
100
200
400
800
...

After fix:
100
100
100
100
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have already passed the absolute point in time (Z_TICKS_ABS(ticks) < sys_clock_tick_get_32()), we should set ticks_to_wait to 0, otherwise we are waiting when we should not be waiting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment. Fixed it

Fix k_sleep implementation for no multi-threading mode.

Absolute value of timeout expiration was fed to the k_busy_wait()
function instead of delta value. That caused bug like incrementing of
sleep time in geometric progression (while actual function argument is
constant) during program running.

Signed-off-by: Mikhail Kushnerov <[email protected]>
@mmahadevan108
Copy link
Contributor

@peter-mitsis , is this a fix that is needed for the upcoming 4.0 release?

@peter-mitsis
Copy link
Contributor

@mmahadevan108 - I think this would be worthwhile for 4.0.

@dkalowsk
Copy link
Contributor

@KushnerovMikhail or @peter-mitsis can you please file an Issue so it can be tracked to a detail in the release-notes?

@JarmouniA JarmouniA added this to the v4.0.0 milestone Nov 11, 2024
@JarmouniA JarmouniA added the bug The issue is a bug, or the PR is fixing a bug label Nov 11, 2024
uint32_t curr_ticks = sys_clock_tick_get_32();

if (Z_TICK_ABS(ticks) > curr_ticks) {
ticks_to_wait = Z_TICK_ABS(ticks) - curr_ticks;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are they possible to be negative number?

(Just a question.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really understand question.

If you talking about Z_TICK_ABS(ticks), than it can. But negative values are caught by previous if.
If you talking about ticks_to_wait, I think there is no meaning in waiting negative time. So, I make it unsigned.
sys_clock_tick_get_32(), of course, can not be negative.

@mmahadevan108 mmahadevan108 modified the milestones: v4.0.0, v4.1.0 Nov 12, 2024
@mmahadevan108
Copy link
Contributor

Moving the milestone to 4.1. Please add the backport-v4.0 label if this is critical for 4.0

@nashif nashif merged commit a995d9d into zephyrproject-rtos:main Nov 16, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Kernel bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kernel: k_sleep bug in no multi-threading mode