Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: fix k_sleep in no multi-threading mode #80979

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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
...

} else {
expected_wakeup_ticks = Z_TICK_ABS(ticks);
/* ticks is absolute timeout expiration */
ticks_to_wait = Z_TICK_ABS(ticks) - sys_clock_tick_get_32();
Copy link
Collaborator

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
Collaborator

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

@peter-mitsis
Copy link
Collaborator

@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
Collaborator

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
Collaborator

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

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
10 participants