Cpu pressure#9
Conversation
|
@claude review |
Claude review of PR #9 (dcdf8a8)<!-- claude-pr-review --> This PR extends the existing memory pressure monitoring infrastructure in sd-event and PID 1 to also support CPU and IO pressure, using Linux's PSI (Pressure Stall Information) interface. The refactoring approach is clean — the memory-pressure-specific code is generalized to work with a The PR is well structured across 7 commits:
Overall the code quality is high and the generalization is done consistently. Suggestions
Nits
<!-- claude-pr-review --> |
|
@claude review |
1 similar comment
|
@claude review |
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -2110,7 +2116,8 @@ static int unit_update_cgroup( | |||
| * that PID 1 runs before init.scope is even set up. Hence, whenever init.scope is realized, let's | |||
| * try to open the memory pressure interface anew. */ | |||
There was a problem hiding this comment.
Claude: nit: Stale comment: "try to open the memory pressure interface anew" should be updated to reflect that this now sets up all pressure resource types (memory, CPU, and IO), e.g. "try to open the pressure interfaces anew".
| for (;;) { | ||
| _cleanup_close_ int fd = -EBADF; | ||
|
|
||
| fd = open("/var/tmp/.io-pressure-test", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0600); |
There was a problem hiding this comment.
Claude: nit: /var/tmp/.io-pressure-test is created by the IO pressure child process but never cleaned up after the test (e.g., in outro() or via _cleanup_). Consider adding (void) unlink("/var/tmp/.io-pressure-test"); to the test cleanup.
|
@claude review |
| {{type}}.MemoryPressureWatch, config_parse_memory_pressure_watch, 0, offsetof({{type}}, cgroup_context.pressure_watch[PRESSURE_MEMORY]) | ||
| {{type}}.CPUPressureThresholdSec, config_parse_sec, 0, offsetof({{type}}, cgroup_context.pressure_threshold_usec[PRESSURE_CPU]) | ||
| {{type}}.CPUPressureWatch, config_parse_memory_pressure_watch, 0, offsetof({{type}}, cgroup_context.pressure_watch[PRESSURE_CPU]) | ||
| {{type}}.NFTSet, config_parse_cgroup_nft_set, NFT_SET_PARSE_CGROUP, offsetof({{type}}, cgroup_context) |
There was a problem hiding this comment.
Claude: nit: config_parse_memory_pressure_watch is reused for CPUPressureWatch and IOPressureWatch, but the function name still says "memory". Consider renaming it to config_parse_pressure_watch for consistency, since it now handles all pressure resource types.
daandemeyer
force-pushed
the
cpu-pressure
branch
2 times, most recently
from
791ae6d to
a01d709
Compare
daandemeyer
force-pushed
the
cpu-pressure
branch
3 times, most recently
from
acaf3f0 to
9f7043b
Compare
Works the same way as memory pressure notifications. Code is refactored to work on enum arrays to reduce duplication.
Make the doc more generic and mention all pressure types, not just memory.
daandemeyer
force-pushed
the
cpu-pressure
branch
from
9f7043b to
acb2b96
Compare
sd_journal_get_data() can return a MESSAGE data object whose payload does not start with "MESSAGE=", e.g. when the journal file is corrupted. Instead of aborting the whole process, log and skip over such an entry like we do for other bad/missing fields. [ 87.287390] post.sh[1619]: + journalctl -q -o short-monotonic --grep 'didn'\''t pass validation' [ 87.287844] post.sh[1620]: + grep -v test-varlink-idl [ 87.325676] post.sh[1619]: Assertion 'message = startswith(message, "MESSAGE=")' failed at src/journal/journalctl-show.c:261, function show(). Aborting. #0 0x00007fb47b49a29c n/a (libc.so.6 + 0x9a29c) #1 0x00007fb47b43e7d0 raise (libc.so.6 + 0x3e7d0) #2 0x00007fb47b425681 abort (libc.so.6 + 0x25681) #3 0x00007fb47b8a1ace log_assert_failed (libsystemd-shared-261~rc2.so + 0xa1ace) #4 0x000055f8e1ef9ddb show (journalctl + 0xcddb) #5 0x000055f8e1efa6ee action_show (journalctl + 0xd6ee) #6 0x000055f8e1ef3c20 run (journalctl + 0x6c20) #7 0x00007fb47b427741 n/a (libc.so.6 + 0x27741) #8 0x00007fb47b427879 __libc_start_main (libc.so.6 + 0x27879) #9 0x000055f8e1ef4915 _start (journalctl + 0x7915) Co-developed-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
No description provided.