Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for querying scoped fields #53

Open
bsteinb opened this issue Jan 5, 2024 · 2 comments
Open

Support for querying scoped fields #53

bsteinb opened this issue Jan 5, 2024 · 2 comments

Comments

@bsteinb
Copy link
Contributor

bsteinb commented Jan 5, 2024

The current implementation of Device::field_values_for is too limiting. The caller can only pass in a slice of field IDs to populate nvmlFieldValue_t::fieldId while all other struct members are set to zero, while nvmlDeviceGetFieldValues actually also uses nvmlFieldValue_t::scopeId as input when querying several fields such as NVLink remote IDs, NVLink ECC counters, or power draw and power limits.

I would like to contribute a function that allows passing in both fieldId and scopeId and was wondering whether that should replace Device::field_values_for or be a separate function, e.g. Device::scoped_field_values_for.

@Cldfire
Copy link
Owner

Cldfire commented Feb 10, 2024

Thanks for the heads up, looks like scopeId was added after I initially created that wrapper 🙂. We should definitely take that into account.

What do you think about changing:

pub fn field_values_for(
    &self,
    id_slice: &[FieldId],
) -> Result<Vec<Result<FieldValueSample, NvmlError>>, NvmlError> {

to:

pub fn field_values_for(
    &self,
    requests_slice: &[FieldValueRequest],
) -> Result<Vec<Result<FieldValueSample, NvmlError>>, NvmlError> {

Where FieldValueRequest is:

/// Specify a field ID and an optional scope ID for requesting data samples
/// from a device.
///
/// Used in [`crate::struct_wrappers::device::FieldValueSample`] and
/// [`crate::device::Device::field_values_for()`].
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct FieldValueRequest {
    /// Populate this newtype with the constants `nvml_wrapper::sys_exports::field_id::*`.
    pub id: FieldId,
    /// Optionally populate this with a `scopeId` appropriate for the associated [`FieldId`].
    ///
    /// See NVIDIA's field ID constant docs (`NVML_FI_*`) to understand what scope
    /// IDs may be valid for which field IDs.
    pub scope_id: Option<ScopeId>,
}

impl FieldValueRequest {
    pub fn id(id: FieldId) -> Self {
        Self { id, scope_id: None }
    }

    pub fn id_with_scope(id: FieldId, scope_id: ScopeId) -> Self {
        Self {
            id,
            scope_id: Some(scope_id),
        }
    }
}

@bsteinb
Copy link
Contributor Author

bsteinb commented Feb 22, 2024

That makes sense. I have submitted PR #55 which changes field_values_for to accept an Iterator of FieldValueRequests. I have also changed FieldValueSample to contain the ScopeId as well to avoid ambiguities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants