scripts: add sleep-accuracy measurement tool#580
scripts: add sleep-accuracy measurement tool#580askervin wants to merge 5 commits intocontainers:mainfrom
Conversation
f179393 to
9d19fb5
Compare
|
@askervin Shouldn't we just review this and get it merged ? |
| FILE *f = NULL; | ||
| while (1) { | ||
| sprintf(disable_filename, "/sys/devices/system/cpu/cpu%d/cpuidle/state%d/disable", cpu, state); | ||
| FILE *f = fopen(disable_filename, "w"); |
There was a problem hiding this comment.
nit: Since we're _GNU_SOURCE, I think we could avoid buffered I/O altogether and just use open and dprintf here.
| FILE *f = NULL; | ||
|
|
||
| sprintf(freq_filename, "/sys/devices/system/cpu/cpu%d/cpufreq/scaling_max_freq", cpu); | ||
| f = fopen(freq_filename, "w"); |
There was a problem hiding this comment.
nit: ditto here... we could avoid buffered I/O with _GNU_SOURCE.
| // Wait until there is more than single CPU to toggle again. | ||
| // This prevents keep setting main thread's CPU affinity to same CPU in a loop. | ||
| while (toggle_cpu0 != -1) { | ||
| delay(toggle_cpu_interval_ns); | ||
| } |
There was a problem hiding this comment.
Is the idea here that if we only have a single CPU (cpu0) then at the end of the first iteration, we stay looping here forever. So basically doing the same as we would do without having this extra while, but avoiding the now unnecessary set_cpu_affinity() ?
| options.cpus[options.cpu_count++][1] = atoi(slash + 1); | ||
| } else { | ||
| options.cpus[options.cpu_count++][0] = atoi(token); | ||
| options.cpus[options.cpu_count - 1][1] = -1; // indicate single CPU pinning |
There was a problem hiding this comment.
This looks a bit confusing. Isn't the intention to set cpus[options.cpu_count][0] to the first part to value A and cpus[options.cpu_count][1] to either the value following '/' or -1 if none? So the latter could also be [options.cpu_count][0] = atoi(token) and [options.cpu_count++][1] = -1? BTW, should it also be checked that atoi() returns useful values and not perhaps -42, and would strtoul() be a better choice?
| char *token = strtok(argv[++i], ","); | ||
| while (token && options.polprio_count < MAX_COMB) { | ||
| char *slash = strchr(token, '/'); | ||
| if (slash) { |
There was a problem hiding this comment.
If there is no slash, the option is silenty ignored?
There was a problem hiding this comment.
Pull request overview
Adds a standalone C tool for measuring nanosleep() latency/accuracy across CPU affinity, scheduling policy/priority, cpuidle, and cpufreq configurations.
Changes:
- Introduces
sleep-accuracy.cimplementing parameter sweeps and percentile reporting for sleep latency. - Adds a simple
Makefileto build/clean the tool.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| scripts/testing/sleep-accuracy/sleep-accuracy.c | Implements the sleep-accuracy measurement tool and sysfs-based CPU configuration helpers. |
| scripts/testing/sleep-accuracy/Makefile | Adds build and clean targets for the tool. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #define uint64_t u_int64_t | ||
|
|
There was a problem hiding this comment.
Redefining uint64_t is unsafe and can break compilation/ABI (and int64_t is used without including its proper definition). Remove the #define uint64_t u_int64_t and include <stdint.h> instead, then use the standard uint64_t/int64_t types.
| #define uint64_t u_int64_t | |
| #include <stdint.h> |
| void delay(uint64_t ns) { | ||
| struct timespec req, rem; | ||
| req.tv_sec = ns / NS_PER_SEC; | ||
| req.tv_nsec = ns % NS_PER_SEC; | ||
| while (nanosleep(&req, &rem) == -1) { | ||
| req = rem; // continue sleeping for the remaining time if interrupted | ||
| } | ||
| } |
There was a problem hiding this comment.
delay() retries on any nanosleep() failure; if the error is not EINTR, rem may be uninitialized and this can spin or behave incorrectly. Check errno == EINTR to continue; otherwise perror and exit/return an error.
| void parse_options(int argc, char *argv[]) { | ||
| // Default values | ||
| options.cpu_count = 0; | ||
| options.polprio_count = 0; | ||
| options.busy_count = 0; | ||
| options.sleep_count = 0; | ||
| options.toggle_count = 0; | ||
| options.iterations = 1000; | ||
| options.repeats = 1; | ||
|
|
||
| options.polprio[options.polprio_count][0] = 0; // Default policy OTHER | ||
| options.polprio[options.polprio_count++][1] = 0; // Default priority 0 | ||
|
|
||
| options.cpuidle_minmax[options.cpuidle_count][0] = 0; // Default cpuidle min state | ||
| options.cpuidle_minmax[options.cpuidle_count++][1] = 99; // Default cpuidle max state | ||
|
|
||
| options.cpufreq_minmax[options.cpufreq_count][0] = 0; // Default cpufreq min [kHz] | ||
| options.cpufreq_minmax[options.cpufreq_count++][1] = 9999999; // Default cpufreq max [kHz] |
There was a problem hiding this comment.
options.cpuidle_count and options.cpufreq_count are used before being initialized in this function, which can cause out-of-bounds writes. Initialize both counts to 0 alongside the other defaults before writing the default min/max entries.
| void print_latencies(int64_t *latencies) { | ||
| uint64_t total_latency = 0; | ||
| uint64_t max_latency = 0; | ||
| uint64_t sum_squares = 0; | ||
| int64_t iters = options.iterations; | ||
| for (int i = 0; i < iters; i++) { | ||
| total_latency += latencies[i]; | ||
| } | ||
|
|
||
| // Sort latencies for percentile calculation | ||
| qsort(latencies, iters, sizeof(uint64_t), compare_uint64); |
There was a problem hiding this comment.
The latency array is int64_t*, but qsort uses sizeof(uint64_t) and an unsigned comparator (compare_uint64). This can mis-sort data (especially if latencies can be negative) and is also type-inconsistent. Use sizeof(int64_t) and an int64_t comparator. Also, total_latency is unsigned but sums signed values, which will underflow if any latency is negative; use int64_t (or long double) for accumulation.
| double avg_latency = (double)total_latency / iters; | ||
|
|
||
| // Calculate percentiles | ||
| int64_t min = latencies[0]; |
There was a problem hiding this comment.
The latency array is int64_t*, but qsort uses sizeof(uint64_t) and an unsigned comparator (compare_uint64). This can mis-sort data (especially if latencies can be negative) and is also type-inconsistent. Use sizeof(int64_t) and an int64_t comparator. Also, total_latency is unsigned but sums signed values, which will underflow if any latency is negative; use int64_t (or long double) for accumulation.
| void set_cpuidle_minmax(int cpu, int min, int max) { | ||
| char disable_filename[1024]; | ||
| int state = 0; | ||
| FILE *f = NULL; | ||
| while (1) { | ||
| sprintf(disable_filename, "/sys/devices/system/cpu/cpu%d/cpuidle/state%d/disable", cpu, state); | ||
| FILE *f = fopen(disable_filename, "w"); |
There was a problem hiding this comment.
The inner FILE *f shadows the outer f, making the outer variable effectively unused and the final if (f) fclose(f); misleading. Remove the outer f, or reuse it (no shadowing). Also prefer snprintf over sprintf to avoid accidental buffer overflows.
| uint64_t toggle_cpu_interval_ns = 1000000; // default 1 ms | ||
| int toggle_cpu_running = 0; | ||
| void configure_cpu_toggler(int cpu0, int cpu1, int interval_ns) { |
There was a problem hiding this comment.
toggle_cpu_interval_ns is uint64_t, but configure_cpu_toggler() takes interval_ns as int, and toggle_ns is also int even though options.toggle_intervals is int64_t. Larger intervals can truncate, producing incorrect toggling behavior. Use uint64_t/int64_t consistently for intervals throughout.
| uint64_t toggle_cpu_interval_ns = 1000000; // default 1 ms | |
| int toggle_cpu_running = 0; | |
| void configure_cpu_toggler(int cpu0, int cpu1, int interval_ns) { | |
| int64_t toggle_cpu_interval_ns = 1000000; // default 1 ms | |
| int toggle_cpu_running = 0; | |
| void configure_cpu_toggler(int cpu0, int cpu1, int64_t interval_ns) { |
| for (int r = 0; r < options.repeats; r++) { | ||
|
|
||
| for (int toggle_idx = 0; toggle_idx < options.toggle_count; toggle_idx++) { | ||
| int toggle_ns = options.toggle_intervals[toggle_idx]; |
There was a problem hiding this comment.
toggle_cpu_interval_ns is uint64_t, but configure_cpu_toggler() takes interval_ns as int, and toggle_ns is also int even though options.toggle_intervals is int64_t. Larger intervals can truncate, producing incorrect toggling behavior. Use uint64_t/int64_t consistently for intervals throughout.
| int toggle_ns = options.toggle_intervals[toggle_idx]; | |
| int64_t toggle_ns = options.toggle_intervals[toggle_idx]; |
|
|
||
| measure(options.busy_times[b_idx], options.sleep_times[s_idx], latencies); | ||
| // print measurement parameters and results | ||
| printf("%d %d %d %ld %d %d %d %d %d %d %ld %ld ", r + 1, |
There was a problem hiding this comment.
%ld is not portable for int64_t (it depends on whether long is 64-bit). Use <inttypes.h> with PRIi64/PRIu64 (or cast to long long and use %lld) to avoid incorrect output on ILP32 platforms.
| @@ -0,0 +1,5 @@ | |||
| sleep-accuracy: sleep-accuracy.c | |||
| gcc -O2 -o $@ $< -lm | |||
There was a problem hiding this comment.
Consider using $(CC) instead of hardcoding gcc, and add basic warnings (e.g., -Wall -Wextra) to catch issues like the type mismatches and unused variables in this file.
| gcc -O2 -o $@ $< -lm | |
| $(CC) -O2 -Wall -Wextra -o $@ $< -lm |
9d19fb5 to
80b29e0
Compare
No description provided.