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: Add k_thread_runtime_stats_is_enabled function #80450

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rruuaanng
Copy link
Collaborator

@rruuaanng rruuaanng commented Oct 25, 2024

Add 'k_thread_runtime_stats_is_enabled' to 'usage.c', which's used to check whether runtime statistics collection is enabled for a thread.

It can determine whether the thread has enabled runtime statistics collection when the program is running. It allows users to add conditional statements based on this function when using it.

for example:

void f()
{
    // Assume there is a thread id
    if (k_thread_runtime_stats_is_enabled(id)) {
        /* do something */
    }
    ...
}

And it can provide a complete support when similar operations are needed. for example enable, disable, is_enabled.
For example, its use in the implementation of CLOCK_THREAD_CPUTIME_ID actually lacks k_thread_runtime_stats_is_enabled, which makes the entire function impossible to implement safely. It is necessary.

@rruuaanng rruuaanng force-pushed the gh80363 branch 2 times, most recently from fc52903 to 075a033 Compare October 25, 2024 15:12
@rruuaanng rruuaanng changed the title kernel: Add k_thread_runtime_stats_enabled function kernel: Add k_thread_runtime_stats_is_enabled function Oct 25, 2024
@rruuaanng rruuaanng marked this pull request as ready for review October 25, 2024 15:28
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Oct 26, 2024
@zephyrbot zephyrbot requested a review from ycsin October 26, 2024 10:40
@rruuaanng rruuaanng force-pushed the gh80363 branch 2 times, most recently from 70b4aa0 to 04e6d25 Compare October 26, 2024 12:02
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Thanks for all of the POSIX clock work.

Just a question / suggestion regarding runtime function call overhead.

include/zephyr/kernel.h Outdated Show resolved Hide resolved
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Just some suggestions around CONFIG_POSIX_THREAD_CPUTIME

lib/posix/options/clock.c Outdated Show resolved Hide resolved
lib/posix/options/clock.c Outdated Show resolved Hide resolved
lib/posix/options/clock.c Outdated Show resolved Hide resolved
@rruuaanng
Copy link
Collaborator Author

I've made change, Please riewer again!

lib/posix/options/clock.c Outdated Show resolved Hide resolved
peter-mitsis
peter-mitsis previously approved these changes Oct 28, 2024
Copy link
Collaborator

@peter-mitsis peter-mitsis left a comment

Choose a reason for hiding this comment

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

Kernel side seems reasonable to me.

ycsin
ycsin previously approved these changes Oct 29, 2024
@cfriedt
Copy link
Member

cfriedt commented Nov 7, 2024

My personal understanding of this is that the test should be submitted with the new function, and make sure that they can be synchronized when rolling back later.

That's one way to do it, but in Zephyr we almost always separate code and test commits.

@rruuaanng
Copy link
Collaborator Author

I've made change :) @cfriedt

cfriedt
cfriedt previously approved these changes Nov 7, 2024
ycsin
ycsin previously approved these changes Nov 7, 2024
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

Apart from the comments below, the commits should be reordered such that kernel: Add k_thread_runtime_stats_is_enabled function comes before posix: Add posix CLOCK_THREAD_CPUTIME_ID support.

tests/posix/common/src/clock.c Outdated Show resolved Hide resolved
include/zephyr/kernel.h Outdated Show resolved Hide resolved
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Forgot I had one pending comment 😬

include/zephyr/kernel.h Outdated Show resolved Hide resolved
@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Nov 13, 2024

This PR has been deferred to 4.1. Maybe I won't be in such a hurry to handle it. I will prepare a PR for the implementation of DT_FOREACH_ANCESTOR today. Stay tuned :)

@rruuaanng
Copy link
Collaborator Author

I've change them, please review again :)
@cfriedt @stephanosio

@rruuaanng
Copy link
Collaborator Author

CI throws. Emmm, I don't know :(

dts.cmake

warning: STDOUT_CONSOLE (defined at lib/libc/Kconfig:135) was assigned the value 'y' but got the
value 'n'. Check these unsatisfied dependencies: CONSOLE_HAS_DRIVER (=n). See
http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_STDOUT_CONSOLE and/or look up
STDOUT_CONSOLE in the menuconfig/guiconfig interface. The Application Development Primer, Setting
Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful
too.

Parsing /__w/zephyr/zephyr/Kconfig
Loaded configuration '/__w/zephyr/zephyr/boards/m5stack/m5stack_cores3/m5stack_cores3_appcpu_defconfig'
Merged configuration '/__w/zephyr/zephyr/samples/synchronization/prj.conf'
Merged configuration '/__w/zephyr/zephyr/twister-out/m5stack_cores3_esp32s3_appcpu/samples/synchronization/sample.kernel.synchronization/zephyr/misc/generated/extra_kconfig_options.conf'
Configuration saved to '/__w/zephyr/zephyr/twister-out/m5stack_cores3_esp32s3_appcpu/samples/synchronization/sample.kernel.synchronization/zephyr/.config'
Kconfig header saved to '/__w/zephyr/zephyr/twister-out/m5stack_cores3_esp32s3_appcpu/samples/synchronization/sample.kernel.synchronization/zephyr/include/generated/zephyr/autoconf.h'
-- Found GnuLd: /opt/toolchains/zephyr-sdk-0.17.0/xtensa-espressif_esp32s3_zephyr-elf/xtensa-espressif_esp32s3_zephyr-elf/bin/ld.bfd (found version "2.38") 
-- The C compiler identification is GNU 12.2.0
-- The CXX compiler identification is GNU 12.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /opt/toolchains/zephyr-sdk-0.17.0/xtensa-espressif_esp32s3_zephyr-elf/bin/xtensa-espressif_esp32s3_zephyr-elf-gcc
-- Espressif HAL path: /__w/zephyr/modules/hal/espressif
CMake Error at /__w/zephyr/zephyr/cmake/modules/extensions.cmake:4178 (message):
  dt_reg_addr(image_off ...) missing required argument: PATH
Call Stack (most recent call first):
  /__w/zephyr/zephyr/soc/espressif/esp32s3/CMakeLists.txt:78 (dt_reg_addr)

@cfriedt
Copy link
Member

cfriedt commented Nov 19, 2024

@rruuaanng - this seems like an unrelated issue in CI. You might need to rebase once it's resolved on main.

ycsin
ycsin previously approved these changes Nov 27, 2024
Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

refresh +1 while I'm here before the CI passes

include/zephyr/kernel.h Outdated Show resolved Hide resolved
@cfriedt
Copy link
Member

cfriedt commented Nov 27, 2024

Probably needs a rebase. qemu_arc_em was doing something kind of wacky, so I made a pr to disable it for the test that's failing.

kernel/usage.c Outdated Show resolved Hide resolved
Add 'k_thread_runtime_stats_is_enabled' to 'usage.c', which's
used to check whether runtime statistics collection is
enabled for a thread.

Signed-off-by: James Roy <[email protected]>
Add test for 'k_thread_runtime_stats_is_enabled(tid)'.

Signed-off-by: James Roy <[email protected]>
Add posix 'CLOCK_THREAD_CPUTIME_ID' support to measure the
total CPU execution time of the current thread.

Signed-off-by: James Roy <[email protected]>
Add test for 'clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts)'.

Signed-off-by: James Roy <[email protected]>
@rruuaanng
Copy link
Collaborator Author

Ehm... There are really many errors in the unittest

RunID: f2a3ca8e6d3fa1c0d4a920a1812b95e8
PROJECT EXECUTION FAILED

INFO    - /__w/zephyr/zephyr/twister-out/qemu_x86_64_atom/tests/kernel/smp_boot_delay/kernel.multiprocessing.smp_boot_delay.minimallibc/handler.log
INFO    - 4 Iteration:
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue

INFO    - 698/698 qemu_x86_64/atom          kernel.multiprocessing.smp_boot_delay.minimallibc  FAILED Testsuite failed (qemu 1.354s)
INFO    - /__w/zephyr/zephyr/twister-out/qemu_x86_64_atom/tests/kernel/smp_boot_delay/kernel.multiprocessing.smp_boot_delay.minimallibc/handler.log
ERROR   - SeaBIOS (version zephyr-v1.0.0-0-g31d4e0e-dirty-20200714_234759-fv-az50-zephyr)
Booting from ROM..
*** Booting Zephyr OS build v4.0.0-1254-gdce4594f9040 ***
Running TESTSUITE smp_boot_delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants