pelt scheduler: add new tests to validate pelt#447
Conversation
|
@vnarapar please fix the workflow issues |
smuppand
left a comment
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.
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>
|
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: |
| log_info "================================================================================" | ||
| log_info "Validates kernel configuration required for PELT (Per-Entity Load Tracking)" | ||
|
|
||
| if ! check_dependencies grep; then |
There was a problem hiding this comment.
uses zgrep but dependency check only checks grep. Also better to log_skip with proper message to console for better debugging.
|
|
||
| log_info "Checking optional Scheduler configurations..." | ||
| for cfg in $OPTIONAL_CONFIGS; do | ||
| if zgrep -qE "^${cfg}=(y|m)" /proc/config.gz 2>/dev/null; then |
There was a problem hiding this comment.
Please use check_optional_config function from functestlib.sh
| log_warn "/proc/config.gz not found - kernel config checks will be skipped" | ||
| log_warn "Ensure CONFIG_IKCONFIG and CONFIG_IKCONFIG_PROC are enabled in the kernel" | ||
| else | ||
| CORE_CONFIGS="CONFIG_FAIR_GROUP_SCHED CONFIG_SMP" |
There was a problem hiding this comment.
Keep CONFIG_SMP required and move CONFIG_FAIR_GROUP_SCHED to optional unless the test is specifically validating group-scheduler PELT
| # --------------------------------------------------------------------------- | ||
| log_info "=== debugfs Mount Check ===" | ||
|
|
||
| if ! grep -q "debugfs" /proc/mounts 2>/dev/null; then |
There was a problem hiding this comment.
This only checks whether any debugfs mount exists, not whether debugfs is mounted at /sys/kernel/debug.
If debugfs is mounted somewhere else, the script may continue and then fail on /sys/kernel/debug/sched.
| log_info "================================================================================" | ||
| log_info "Validates the scheduler debugfs interface used to inspect PELT state" | ||
|
|
||
| if ! check_dependencies grep awk cat; then |
There was a problem hiding this comment.
include id, mount, basename
|
|
||
| # Validate ordering: latency >= min_granularity (CFS invariant) | ||
| lat_path="$SYSCTL_DIR/sched_latency_ns" | ||
| gran_path="$SYSCTL_DIR/sched_min_granularity_ns" |
There was a problem hiding this comment.
Current script still fails if these are missing:
sched_min_granularity_ns
sched_latency_ns
sched_wakeup_granularity_ns
run.sh
These sysctls can be kernel-version/config dependent. The test can fail a valid target just because those tunables are not exposed.
Recommended fix: Either document these as mandatory for this test, or make missing tunables warning-only:
log_warn " $tunable not found at $path (kernel/config dependent)"
Do not set pass="false" for absence unless this test is explicitly enforcing those knobs
2 participants
This PR adds new testcases to validate PELT scheduler