Repository navigation
Bugfix in dcgmlib: Check per-field nvmlReturn for DCGM_FI_DEV_MEMORY_TEMP_CELSIUS - #311
Open
brianwolfe wants to merge 2 commits into
Open
brianwolfe wants to merge 2 commits into
brianwolfe wants to merge 2 commits into
Conversation
nvmlDeviceGetFieldValues returns NVML_SUCCESS when it populates any entry in
the values array. In nvml.h, it states that the caller must check each
value's own nvmlReturn before reading it, because value is undefined
unless that per-field status is NVML_SUCCESS.
The DCGM_FI_DEV_MEMORY_TEMP_CELSIUS handler in BufferOrCacheLatestGpuValue
checked only the call's return and then read value.value.uiVal. On a GPU with
no memory temperature sensor NVML succeeds and sets the per-field status to
NVML_ERROR_NOT_SUPPORTED, leaving value at its zero-initialized state, so the
handler cached a literal 0 as a real measurement rather than
DCGM_INT64_NOT_SUPPORTED. 0 C.
This is inside the valid range for a temperature, so an unsupported field is
then indistinguishable from a valid reading for every consumer of the field.
I found this on GCP using an L4 node. Calling NVML directly on an L4 (driver
580.173.02), with no DCGM in the path:
NVML_FI_DEV_MEMORY_TEMP (id 82)
call nvmlDeviceGetFieldValues -> NVML_SUCCESS
value.nvmlReturn -> NVML_ERROR_NOT_SUPPORTED
value.uiVal -> 0
nvidia-smi -q -d TEMPERATURE reports "Memory Current Temp : N/A" on the same
card, and NVIDIA/dcgm-exporter#460 reports the field reading 0 on a 3090, a
4070 Ti Super and an L40S.
Promote the per-field status into nvmlReturn ahead of the existing error check
so the handler stores the blank that path already produces. This matches
ActuallyUpdateGpuFieldValues and the other single-field call sites in this
file, which check the per-value status today.
After making this change and testing it on the same node type, dcgm-exporter
no longer emits the invalid memory temperature time series.
This error mode also impacts DCGM_FI_DEV_BOARD_POWER_RAW_WATTS since it uses
the same pattern in the same function. I did not immediately see a way to check
that on real hardware, so I did not change that access.
Signed-off-by: Brian Wolfe <wolfe.br@gmail.com>
This test seems a bit excessive, but I don't have access to the test infrastructure for this, so I kind of had to make do with my local setup. If someone can update the documentation for building and testing this library, I can try another stab at getting a reasonable test setup. As it is, this test at least catches the case where we get a NVML_SUCCESS from the function, but not for the memory sensor read itself, the case I was seeing on real hardware.
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.
Summary
On L4 nodes, and likely other GPUs that do not support NVML_FI_DEV_MEMORY_TEMP, DCGM reports an invalid "0" value for the temperature. This was raised as a comment here, potentially impacting other cards. I don't have those cards to test.
nvmlDeviceGetFieldValues returns NVML_SUCCESS when it populates any entry in the values array. In nvml.h, it states that the caller must check each value's own nvmlReturn before reading it, because value is undefined unless that per-field status is NVML_SUCCESS.
nvmlDeviceGetFieldValuesAPInvml.hstates the requirement in thenvmlFieldValue_ststruct:and in the function call:
So NVML_SUCCESS means that "the array was populated", not that any values were populated.
The DCGM_FI_DEV_MEMORY_TEMP_CELSIUS handler in BufferOrCacheLatestGpuValue checked only the call's return and then read value.value.uiVal. On a GPU with no memory temperature sensor NVML succeeds and sets the per-field status to NVML_ERROR_NOT_SUPPORTED, leaving value at its zero-initialized state, so the handler cached a literal 0 as a real measurement rather than DCGM_INT64_NOT_SUPPORTED.
What is the Result
This results in a valid-looking temperature of 0 C. This is inside the valid domain for a temperature, so an unsupported field is then indistinguishable from a valid reading for every consumer of the field. This shows up when monitoring GPUs with dcgm-exporter on L4 GPUs on GCP.
Using such an L4 GCP node, I reproduced the underlying behavior. Calling NVML directly on an L4 (driver 580.173.02), with no DCGM in the path:
nvidia-smi recognizes the missing sensor on the same card:
Fix
Check the per-field nvmlReturn for DCGM_FI_DEV_MEMORY_TEMP_CELSIUS.
I made this fix. Afterwards, the previous test scripts using python and using dcgm-exporter showed the expected behavior (missing the time series, not 0 C).
I did not have an obvious way to test the other usages of
nvmlDeviceGetFieldValues, but it looks likeDCGM_FI_DEV_BOARD_POWER_RAW_WATTScould have the same error. I did not verify or fix. I didn't have hardware to test that failure on so left it as-is.