Repository navigation
Fix temperature threshold units and export the full JSON snapshot - #557
Merged
Merged
Conversation
gpuinfo_static_info did not document the unit of its temperature thresholds, and the backends disagreed: NVIDIA, Iluvatar, Enflame, Ascend and Tenstorrent report degrees Celsius, AMDGPU stored the millidegrees read from temp1_crit/temp1_emergency verbatim, and MetaX stored the values from mxSmlGetTemperatureInfo as is. MXSML documents them as degrees Celsius enlarged by 100 times, which is why the backend already divides the hotspot reading by 100; the thresholds were not converted, so the hardcoded slowdown value of 9500 actually meant 95 degrees. The only consumer, draw_temp_color(), compares the slowdown threshold with gpu_temp, which is in degrees Celsius. On AMDGPU that comparison could never be true, so a GPU close to its critical temperature was still drawn green instead of yellow or red. Document the unit in the struct and convert the AMDGPU and MetaX values when they are stored. Tenstorrent and the legacy radeon backend already divided by 1000, and the remaining backends already report degrees.
`nvtop -s` printed only a subset of what nvtop already collects, so the JSON output could not be used to build a dashboard without falling back to the interactive UI or to vendor tools. Print the fields that were missing: the PCI address, the static topology (integrated_graphics, encode_decode_shared, max_pcie_gen, max_pcie_link_width, n_shared_cores, l2cache_size, n_exec_engines), the maximum clocks, the slowdown and shutdown temperature thresholds, fan RPM, maximum power draw, the PCIe link generation, width and throughput, the effective load rate and the ECC counters. The output stays a superset of what it was: existing keys keep their name, type and position relative to each other, and every new field follows the existing convention of printing null when the backend did not report it. `pci` is null for devices that are not on a PCI bus. The temperatures are converted to Fahrenheit on request, like the temperature field already was. Since the new threshold fields need the same conversion as the meter, it is factored into one integer-only helper, celsius_to_fahrenheit(), instead of duplicating the floating point formula.
The snapshot writer escaped only newline, backspace, form feed, carriage return, tab, backslash and double quote. Any other control character in a process command line -- ESC is common enough, and argv can hold anything -- was written raw, which is not valid JSON: jq happens to accept it, while python, go and JavaScript reject the document. Bytes that are not valid UTF-8 break the encoding requirement in the same way. Add print_json_string(), which escapes every control character as \u00XX, copies well formed UTF-8 sequences untouched and replaces stray bytes with U+FFFD, and use it for the command line, the user name and the device name, architecture and memory type, so no string field can break the document.
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
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.
nvtop -sprinted only a subset of what nvtop already collects, so its JSON could not drive a dashboard without falling back to the interactive UI or to vendor tools. This series closes that gap, and fixes the bugs that surfaced once those fields were inspected.It supersedes #445, which asked for the same thing but had grown into a branch that also carried unrelated amdgpu fan/PCIe work and no longer applies.
Commits
amdgpu, metax: store the temperature thresholds in degrees Celsiusgpuinfo_static_infonever documented the unit oftemperature_slowdown_threshold/temperature_shutdown_threshold, and the backends disagreed: NVIDIA, Iluvatar, Enflame, Ascend, Tenstorrent and PPU already store degrees Celsius, AMDGPU stored the millidegrees fromtemp1_crit/temp1_emergencyverbatim, and MetaX storedmxSmlGetTemperatureInfo()'s hundredths of a degree as is (its hardcoded9500actually meant 95 °C).The only consumer,
draw_temp_color(), compares the slowdown threshold withgpu_temp, which is in Celsius. On AMDGPU that comparison could never be true, so a card at its critical temperature was still drawn green. Document the unit and convert at the two backends that got it wrong. Tenstorrent and the legacy radeon backend already divided by 1000.interface: export the remaining per-device fields in snapshot modeAdds
pci,integrated_graphics,encode_decode_shared,max_pcie_gen,max_pcie_link_width,n_shared_cores,l2cache_size,n_exec_engines,gpu_clock_max,mem_clock_max,temp_slowdown_threshold,temp_shutdown_threshold,fan_rpm,power_draw_max,pcie_link_gen,pcie_link_width,pcie_rx,pcie_tx,effective_load_rate,ecc_correctedandecc_uncorrected.Existing keys keep their name, type, position and raw-byte memory representation, and every new field follows the established convention of printing
nullwhen the backend did not report it.pciisnullfor devices that are not on a PCI bus. The Celsius to Fahrenheit conversion is now one integer-only helper (F = C * 9 / 5 + 32, rounded by adding 2 before the division) shared with the temperature meter, instead of the floating point formula being duplicated.interface: emit valid JSON for arbitrary process stringsThe writer escaped only
\n \b \f \r \t \" \\. Any other control character in a process command line — ESC is easy enough to find, andargvcan hold anything — was emitted raw, which strict parsers reject (jqaccepts it, CPython/Go/JavaScript do not). Invalid UTF-8 broke conforming readers the same way.print_json_string()now escapes every control character as\u00XX, copies well formed UTF-8 sequences untouched and replaces stray bytes with U+FFFD, and is used forcmdline,user,device_name,device_architectureandmemory_type.Testing
ctest28/28 on this machine.python3 json.loads,encoding="utf-8") of-s,-s -P,-s -f,-s -P -fand-d 1 -s.argvcontained\x01, ESC, a Latin-1\xe9, an overlong\xc0\xafsequence and an emoji. Before the escaping commitjson.loadsfailed withInvalid control character at: line 119 column 23; after it, all of them round-trip.temp1_crit=100000,temp1_emergency=105000):"temp_slowdown_threshold": "100C","temp_shutdown_threshold": "105C", and212F/221Fwith-f. Previously those values were stored as100000, so alerts never triggered on AMD.32 + nearbyint(C * 1.8)for every value from 0 to 100 000 °C: 0 differences.utf8_sequence_length()compared against CPython's strict decoder on 369 572 sequences (exhaustive 1- and 2-byte, a 3/4-byte boundary grid and 300 000 random ones): 0 mismatches. Against glibcmbrtowc()over 16 911 360 sequences the only 22 differences are the encodings above U+10FFFF that glibc tolerates and RFC 3629 forbids.Nothing was tested on MetaX hardware; the MetaX change follows the
mxSmlGetTemperatureInfo()contract ("degrees Celsius enlarged by 100 times"), which is also why that backend already divides the hotspot reading by 100.