integration: liquescent (CI only — do not merge) - #5
Closed
richardkiene wants to merge 7 commits into
Closed
Conversation
nvfd only needs NVML, and NVML ships with every NVIDIA driver as libnvidia-ml.so.1. The only reason the CUDA toolkit was required was nvml.h, and installing the toolkit through a distribution package (nvidia-cuda-toolkit on Debian/Ubuntu) can replace or pin the driver on the host. - Add include/nvml_api.h declaring the 17 NVML entry points nvfd calls, with the enum values and struct layouts from nvml.h. Signatures are part of the versioned ABI (_v2 suffixes), so this is stable. - Link by SONAME (-l:libnvidia-ml.so.1) so no libnvidia-ml.so dev symlink is needed either. Compile with -Werror=implicit-function-declaration so an undeclared NVML call is a build error, not a silent implicit int. - nvmlDeviceSetFanControlPolicy is now always compiled in and its result checked, instead of being #ifdef'd on whatever header happened to be installed. Minimum driver is R520. - install.sh: stop installing nvidia-cuda-toolkit; refuse to build if libnvidia-ml.so.1 is not in the linker cache; run the post-install `nvfd list` visibly so an NVML init failure aborts the install instead of being swallowed by `|| true`. - CI: build without the toolkit.
The daemon used to swallow every error: an unparseable config was treated as "all GPUs auto", a missing or corrupt curve fell back to a hardcoded default, an unknown mode fell back to that default too, and a failed nvmlDeviceSetFanSpeed_v2 was ignored so the service sat "active" while logging to stderr every 5 s. A daemon that hung inside NVML was never restarted, leaving the fans pinned at the last speed it set. Daemon: - Every error is fatal. The reason goes to syslog, the fans are reset to auto, and the process exits non-zero so Restart=on-failure takes over. This covers: unreadable config or curve file, manual mode without a valid speed, unknown mode, temperature read failure, and any fan command NVML rejects. - The curve file is re-read every poll, like config.json already was, so `nvfd curve <temp> <speed>` takes effect within 5 s. SIGHUP is kept as a no-op so a stray HUP does not kill the daemon; ExecReload is gone. - sd_notify READY=1 / WATCHDOG=1 / STOPPING=1 via a 40-line AF_UNIX datagram shim (src/notify.c), no libsystemd dependency. The unit is Type=notify with WatchdogSec=30 against a 5 s poll. - StartLimitIntervalSec=60 / StartLimitBurst=5 so persistent failure parks the unit in "failed" instead of restarting forever. Readers: - config_read() distinguishes "no file yet" (empty object) from "file exists but is unusable" (NULL + config_last_error()). config_write_gpu refuses to overwrite a config it could not parse. - curve_load() validates the file: object at top level, integer keys 0-100, integer values 0-100, no duplicate temperatures (which divided by zero in curve_interpolate), at most MAX_CURVE_POINTS, at least one point. curve_read() stays as a wrapper for the TUI. - curve_edit/curve_reset return status instead of void; curve_edit refuses to overwrite an invalid file. CLI: - `nvfd curve` and `nvfd <n> curve` verify the curve loads before switching a GPU into curve mode, so the daemon is never handed a config it will immediately die on. - Exit status is non-zero when a command fails. - The TUI and `nvfd status` exit with the parse error instead of showing a blank config; the curve editor refuses to open on an invalid file rather than overwriting it from the default on save.
daemon_loop (src/main.c prev_managed[MAX_GPU_COUNT]) and the dashboard (src/dashboard.c gpus[MAX_GPU_COUNT], init_mode[MAX_GPU_COUNT][16], init_speed[MAX_GPU_COUNT]) keep per-GPU state in arrays sized by MAX_GPU_COUNT (8) but indexed by device_count, so a host with nine or more GPUs overran them. Fail at gpu_init with a message naming the limit and the constant to raise instead.
Defects:
- The TUI could write mode "curve" with no curve.json and fall back to a
built-in curve, while the daemon died on that same config every 5 s
until StartLimitBurst parked the unit. apply_mode and apply_curve_fans
now load the curve first and exit the TUI with the reason if it is
missing or invalid. curve_default_interpolate has no callers left and
is removed.
- Legacy plain-text migration accepted speeds 0-100 and silently mapped
garbage to "auto"; the daemon rejects speeds below FAN_SPEED_MIN, so a
legacy "20" produced a config it died on forever. Migration now
refuses anything but auto, curve, or a speed in FAN_SPEED_MIN..MAX,
and main treats a migration failure as fatal.
- fan_get_count returned 0 on NVML error, so fan_reset_to_auto reset
zero fans and reported success; daemon_die's "could not be reset"
path never fired. fan_get_count returns -1 on error and
fan_reset_to_auto treats that as a failure.
Hardening:
- manual_speed range-checks the json_int_t before narrowing to int.
- json_load_file uses JSON_REJECT_DUPLICATES for config and curve, so
{"30":30,"30":50} is rejected instead of last-one-wins.
- parse_temperature requires a leading digit: no "+30", no " 30".
- daemon_loop honours WATCHDOG_USEC and refuses to start if WatchdogSec
is under twice the poll interval.
- First poll hands every GPU not in manual/curve mode back to the
driver, so an instance that died uncleanly cannot leave those fans
pinned. A GPU that cannot be reset on that startup sweep is logged
and skipped rather than fatal: this instance never touched it, and a
card without controllable fans must not stop the others being managed.
- Dashboard reports editor_run's error via curve_last_error after
endwin; editor_run no longer prints under curses.
- display_fan_curve distinguishes missing (suggest reset) from invalid
(print the error; do not suggest overwriting it) and returns status.
- display_list_gpus says "fan count unavailable" instead of "-1 fans".
- require_curve is now curve_require in curve.c, shared with
curve_apply_to_gpu.
Review follow-ups: - install.sh ran the now-fatal `nvfd list` after stopping the old service and installing the new binary, so an NVML init failure left a half-upgraded host. Build first, probe with the fresh build/nvfd, and only then stop, install and start. The ldconfig preflight moves above the package install and its result feeds `make LDFLAGS=-L<dir>`, so the linker searches the directory ld.so actually resolves the library from instead of only its defaults. - CI only compiled objects, so neither the -l:libnvidia-ml.so.1 link line nor the declared symbol set was exercised. tests/nvml_stub.c defines every entry point from nvml_api.h with the same signature; CI builds it as a stub libnvidia-ml.so.1 and links nvfd against it. The stub also fails to compile if nvml_api.h drifts. - nvml_api.h comment: Set*FanSpeed_v2 appeared in R515, not R520; nvmlDeviceSetFanControlPolicy in 520.61.05 (backported to 515.105.01). R520 stays the floor.
Author
|
CI passed on 7176d7c (run 33530208357: compile, stub-NVML link, shellcheck). Closing — this branch is deployed directly, never merged into main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge of #1, #2 and #3 at their post-review tips. Exists only so CI compiles and link-tests the combined tree; the fork's
mainstays identical to upstream. Close after the run.