pelt scheduler: add new tests to validate pelt#447
Conversation
|
@vnarapar please fix the workflow issues |
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 @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>
This PR adds new testcases to validate PELT scheduler