pelt scheduler: add new tests to validate pelt - #447
Conversation
|
vnarapar please fix the workflow issues |
Srikanth Muppandam (smuppand)
left a comment
Srikanth Muppandam (smuppand)
left a comment
There was a problem hiding this comment.
Since there are multiple files in the PR, I am providing a consolidated review.
- CI is failing for executable permissions and Shell Lint.
- All run.sh files use
[ -z "$__INIT_ENV_LOADED" ]; please switch to${__INIT_ENV_LOADED:-}. - All run.sh files call check_dependencies but do not check the return value.
- PELT_decay Method 1 reads
/proc/self/schedthrough grep, so it reads the grep process sched data instead of the shell workload. Please read/proc/$$/schedinstead. - PELT_sched_debug checks only whether
/sys/kernel/debugexists, not whether debugfs is mounted. Please validate debugfs via/proc/mounts. - PELT_schedutil uses
-gefor “frequency increased”, making the equal-frequency warning path unreachable. Use-gtfor increase and handle equality separately. - Several scripts write .res and then exit 1. Please exit 0 after writing test FAIL/SKIP results so LAVA can publish the result cleanly.
- PELT_config still has copy-paste “DMA-BUF” log messages.
- Some tunables/sysfs entries are kernel-version/config dependent and should not be hard failures unless explicitly documented.
- YAMLs currently mask cd/run/send-to-lava failures with
|| true. Please remove unnecessary masking once run.sh result handling is fixed.
After these fixes and runtime evidence on at least one target, this can be reviewed again.
ae76072 to
1d30a61
Compare
|
Hi Srikanth Muppandam (@smuppand) , thanks fixed the issues. Have a query wrt Iusse#2. I see that most scripts are using [ -z "$__INIT_ENV_LOADED" ];, any reason to move to ${__INIT_ENV_LOADED:-}. |
because ${var:-} is safe when the variable is unset. In the current script, the code directly expands "$__INIT_ENV_LOADED" before sourcing init_env. Why it matters In normal /bin/sh without set -u, both may work. But if the script is run by a stricter wrapper, CI harness, or future common runner with: set -u then this line can fail: "$__INIT_ENV_LOADED" because the variable may not exist yet. You can get an error like: __INIT_ENV_LOADED: parameter not set Using this form avoids that: "${__INIT_ENV_LOADED:-}" It means: Use $__INIT_ENV_LOADED if it is set; otherwise use empty string. So the test becomes safe even when the variable is unset. Why we use it in repo scripts This pattern is already safer and more portable: if [ -z "${__INIT_ENV_LOADED:-}" ]; then It avoids: |
|
vnarapar, please rebase and apply the small fixes mentioned above. |
This PR adds new testcases to validate PELT scheduler - This validates configs, decay, load tracking, schedutil and different tunables Signed-off-by: Vamsee Narapareddi <vnarapar@qti.qualcomm.com>
|
@ualcomm/qualcomm-linux-testing.triage This pull request has been marked as stale due to 30 days of inactivity and will automatically close after an additional 5 days. |
|
@ualcomm/qualcomm-linux-testing.triage This pull request has been marked as stale due to 30 days of inactivity and will automatically close after an additional 5 days. |
Srikanth Muppandam (smuppand)
left a comment
Srikanth Muppandam (smuppand)
left a comment
There was a problem hiding this comment.
All seven suites duplicate bootstrap, manual .res writes, and pass flags instead of using the repository bootstrap and test_result_init, test_result_record, and test_result_finish.
Result and cleanup behavior diverges across the seven suites, and future framework fixes will not propagate.
Convert every suite to the current repository bootstrap and shared result API. Put reusable PELT probing in a shared helper rather than repeating it across launchers.
| # --------------------------------------------------------------------------- | ||
| # Prerequisite: /proc/schedstat must be present | ||
| # --------------------------------------------------------------------------- | ||
| if [ ! -f /proc/schedstat ]; then |
There was a problem hiding this comment.
/proc/schedstat is not PELT evidence
Rename and scope this as a schedstat activity test, or validate a PELT-specific signal such as se.avg.util_avg for the workload task when the required debug capability is available.
| log_info " Total run_delay : ${total_delay} ns" | ||
| log_info " Total pcount : ${total_pcount}" | ||
|
|
||
| if [ "$total_runtime" -gt 0 ] 2>/dev/null; then |
There was a problem hiding this comment.
false PELT pass criterion. Keep this as an informational schedstat check, or move it to a scheduler-statistics suite. Do not make it the functional pass criterion for PELT.
| if ! grep -qE "debugfs /sys/kernel/debug" /proc/mounts 2>/dev/null; then | ||
| if [ "$(id -u 2>/dev/null)" = "0" ]; then | ||
| log_info "debugfs not mounted - attempting to mount..." | ||
| if mount -t debugfs debugfs /sys/kernel/debug 2>/dev/null; then |
There was a problem hiding this comment.
test-mounted debugfs is never restored. Record whether the test mounted debugfs and unmount it in an EXIT, INT, and TERM cleanup handler only when the test mounted it.
| run: | ||
| steps: | ||
| - REPO_PATH=$PWD | ||
| - cd "$REPO_PATH/Runner/suites/Kernel/Scheduler/PELT_schedstat" || true |
There was a problem hiding this comment.
masked directory failure. cd ... || true suppresses failure to enter the suite directory.
Remove || true and use the standard separate repo-path, cd, invocation, and result-upload steps.
2 participants
This PR adds new testcases to validate PELT scheduler