Skip to content

k_cycle_get_32() API is useless in some Zephyr configurations #16238

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

Closed
pizi-nordic opened this issue May 17, 2019 · 6 comments · Fixed by #16302
Closed

k_cycle_get_32() API is useless in some Zephyr configurations #16238

pizi-nordic opened this issue May 17, 2019 · 6 comments · Fixed by #16302
Assignees
Labels
area: API Changes to public APIs area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@pizi-nordic
Copy link
Collaborator

pizi-nordic commented May 17, 2019

TL;DR:
CONFIG_USERSPACE=y + CONFIG_TIMER_READS_ITS_FREQUENCY_AT_RUNTIME=y = No reference for k_cycle_get_32().

The CONFIG_USERSPACE option introduces strong separation between the application and the
kernel memory. The CONFIG_TIMER_READS_ITS_FREQUENCY_AT_RUNTIME makes the CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define useless leaving the sys_clock_hw_cycles_per_sec() function the only source of system timer frequency, which is needed to interpret data from k_cycle_get_32().

Unfortunately, the sys_clock_hw_cycles_per_sec() function directly reads the kernel memory (z_clock_hw_cycles_per_sec) triggering exception when called from user thread.

Please also note, that sys_clock_hw_cycles_per_sec() is used in multiple inline functions, so other timing-related API also fails with the mentioned configuration.

@pizi-nordic pizi-nordic added bug The issue is a bug, or the PR is fixing a bug area: API Changes to public APIs labels May 17, 2019
@pizi-nordic
Copy link
Collaborator Author

pizi-nordic commented May 17, 2019

Currently this bug in connection with #15230 (which prevents the tests from not using userspace if platform supports it), blocks #16203, which is a fix for #15983.

@pizi-nordic
Copy link
Collaborator Author

FYI: @nashif, @andrewboie, @carlescufi

@andrewboie
Copy link
Contributor

what I think is needed is a new, private z_ prefixed API, like z_sys_runtime_clock_hw_cycles_per_sec() which returns runtime calculated system clock frequency. make it a system call. and then wrap sys_clock_hw_cycles_per_sec to either call that or just return CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC

@pizi-nordic
Copy link
Collaborator Author

I would rather name it k_cycle_freq_get() in order to relate it to k_cycle_get_32(). IMHO on platforms witch clock speed known at compile time, it could be a static inline returning constant value and syscall on the others. What do you think?

@andrewboie
Copy link
Contributor

andrewboie commented May 21, 2019

it could be a static inline returning constant value and syscall on the others. What do you think?

I don't care what you call it, but you can't ifdef its definition.
The script that scans for syscall names in header files doesn't use the preprocessor. That is why I said to leave the ifdefery in the sys_clock_hw_cycles_per_sec() definition

@pizi-nordic
Copy link
Collaborator Author

I don't care what you call it, but you can't ifdef its definition.
The script that scans for syscall names in header files doesn't use the preprocessor.

Ok. I have not known that.

andrewboie pushed a commit to andrewboie/zephyr that referenced this issue May 22, 2019
If the system sets its clock frequency at runtime, this is
stored in a variable that can't be directly read by user
mode. For this case only, add a system call to fetch its
value and modify the definition of
sys_clock_hw_cycles_per_sec() to use it.

Since this is now a system call, store in a temporary variable
inside z_ms_to_ticks(). The syscall overhead only applies
when called from user mode, other contexts are completely
inlined.

Added stub syscall header for mocking framework, to get rid
of inclusion errors.

Fixes: zephyrproject-rtos#16238

Signed-off-by: Andrew Boie <[email protected]>
carlescufi pushed a commit that referenced this issue May 22, 2019
If the system sets its clock frequency at runtime, this is
stored in a variable that can't be directly read by user
mode. For this case only, add a system call to fetch its
value and modify the definition of
sys_clock_hw_cycles_per_sec() to use it.

Since this is now a system call, store in a temporary variable
inside z_ms_to_ticks(). The syscall overhead only applies
when called from user mode, other contexts are completely
inlined.

Added stub syscall header for mocking framework, to get rid
of inclusion errors.

Fixes: #16238

Signed-off-by: Andrew Boie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants