scripts: add sleep-accuracy measurement tool - #580
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.
| 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) { |
|
|
||
| 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
| f = fopen(freq_filename, "w"); | ||
| if (f) { | ||
| fprintf(f, "%d\n", min); | ||
| fflush(f); |
There was a problem hiding this comment.
Matter of taste: setlinebuf() might save on fflush()-es.
bc7c826 to
0fffcc9
Compare
No description provided.