New plugin: gpu_nvml collects NVIDIA GPU stats. #2923

qdbp · 2018-09-10T16:17:23Z

This is a C plugin that efficiently collects various operational parameters of NVIDIA GPUs through direct invocation of the NVML API. The targeted use case is monitoring and debugging hardware status for long-running GPGPU jobs (e.g. a BOINC daemon).

Currently following stats are collected:

fan speed
core temperature and power consumption
memory and streaming multiprocessor clocks
percent GPU and memory loads

The NVML header is provided by CUDA, having which is a build requirement for this module.

ccnifo · 2018-09-11T12:36:23Z

Great work, thanks!
Maybe you should consider nvmlDeviceGetMemoryErrorCounter? We found this metric interesting to spot unexpected hardware errors ending in jobs being killed.

octo

Hi @qdbp,

thank you very much for your pull request! The plugin appears to be straight forward and well written, I mostly have some coding style comments inline.

The lack of license information is a hard blocker for now though.

Thanks again and best regards,
—octo

src/gpu_nvml.c

octo · 2018-10-21T19:54:25Z

src/gpu_nvml.c

+  char *eptr;
+
+  if (strcasecmp(key, config_keys[0]) == 0) {
+    device_ix = strtoul(value, &eptr, 10);


Please declare device_ix here.

https://collectd.org/review-comments#define-variables-on-first-use

octo · 2018-10-21T19:59:04Z

src/gpu_nvml.c

+    device_count = 64;
+  }
+
+  for (int ix = 0; ix < device_count; ix++) {


This compares signed and unsigned ints, causing compiler warnings. Please use unsigned int ix = 0 instead.

octo · 2018-10-21T19:59:35Z

src/gpu_nvml.c

+
+  for (int ix = 0; ix < device_count; ix++) {
+
+    int is_match = ((1 << ix) & conf_match_mask) || (conf_match_mask == 0);


Implicit cast to unsigned int. Please change the variable type to unsigned int to avoid compiler warnings.

octo · 2018-10-21T20:01:17Z

src/gpu_nvml.c

+    nvmlDevice_t dev;
+    TRY(nvmlDeviceGetHandleByIndex(ix, &dev));
+
+    char dev_name[MAX_DEVNAME_LEN + 1];


Initialize right here:

char dev_name[MAX_DEVNAME_LEN + 1] = {0};

octo · 2018-10-21T20:05:52Z

src/gpu_nvml.c

+  unsigned long device_ix;
+  char *eptr;
+
+  if (strcasecmp(key, config_keys[0]) == 0) {


I'd find this easier to read if you repeated the string literal "GPUIndex" here. Alternatively, add a comment to config_keys that its order matters – that is not normally the case.

octo · 2018-10-21T20:06:44Z

src/gpu_nvml.c

+#define TRY(f) TRY_CATCH(f, catch)
+#define TRYOPT(f) TRY_CATCH_OPTIONAL(f, catch)
+
+#define WRAPGAUGE(x) ((value_t){.gauge = (gauge_t)(x)})


If you pass a gauge_t to nvml_submit() and do the conversion there, I think you can avoid this macro.

octo · 2018-10-21T20:09:25Z

src/gpu_nvml.c

+    conf_match_mask |= (1 << device_ix);
+  } else if (strcasecmp(key, config_keys[1])) {
+    if
+      IS_TRUE(value) { conf_mask_is_exclude = 1; }


conf_mask_is_exclude = IS_TRUE(value);

octo · 2018-10-21T20:11:08Z

src/gpu_nvml.c

+  if (strcasecmp(key, config_keys[0]) == 0) {
+    device_ix = strtoul(value, &eptr, 10);
+    if (eptr == value) {
+      ERROR("Failed to parse GPUIndex value \"%s\"", value);


Please prefix error messages with the plugin name, e.g.:

ERROR(PLUGIN_NAME ": Failed …")

octo · 2018-10-21T20:12:39Z

configure.ac

@@ -6735,6 +6788,7 @@ AC_PLUGIN([filecount],           [yes],                     [Count files in dire
 AC_PLUGIN([fscache],             [$plugin_fscache],         [fscache statistics])
 AC_PLUGIN([gmond],               [$with_libganglia],        [Ganglia plugin])
 AC_PLUGIN([gps],                 [$plugin_gps],             [GPS plugin])
+AC_PLUGIN([gpu_nvml],            [$with_cuda],              [NVIDIA GPU plugin])


What is "nvml" standing for? Is this a well-known acronym / abbreviation? I'd find "gpu_nvidia" more descriptive, but I have next to no experience in GPU things ;)

It's the NVIDIA gpu management library. I think claiming just "GPU" is too broad, since there are other vendors and interfaces out there.

rpv-tomsk · 2018-10-23T08:47:29Z

src/gpu_nvml.c

+
+static int nvml_config(const char *key, const char *value) {
+
+  char *eptr;


Please declare variables when they are first used.
So, move this declaration to line 72.

https://collectd.org/review-comments#define-variables-on-first-use

rpv-tomsk · 2018-10-23T08:56:43Z

src/gpu_nvml.c

+  return -1;
+}
+
+static void nvml_submit(const char *plugin_instance, const char *type,


After WRAPGAUGE macro was removed, last arg becomes gauge_t instead of value_t.
Please, rename nvml_submit to nvml_submit_gauge. That is more consistent with exiting practice.

Without such rename, it can lead to mismatch: metric, defined by type, can be of other type than 'gauge'.
If function name will reflect type, what error has chances to not occur.

octo

Hi @qdbp,

thank you very much for your update! I left some suggestions inline, but overall this looks good to go.

I'd like to revisit the plugin name, though: I agree that "gpu" is probably too generic. I think that "gpu_nvidia" would be a good choice, because it conveys to users that this plugin is specific to nvidia GPUs. That the library used for communicating with the GPUs is "nvml" is an implementation detail the user likely doesn't care about.

Best regards,
—octo

octo · 2018-10-25T07:51:03Z

src/collectd.conf.in

+#<Plugin gpu_nvml>
+#   GPUIndex 0
+#   GPUIndex 2
+#   IgnoreSelected 0


I think this should be false instead of 0. The settings here should reflect the default the plugin uses.

octo · 2018-10-25T07:54:49Z

src/gpu_nvml.c

+    unsigned int sm_clk_mhz;
+    TRYOPT(nvmlDeviceGetClockInfo(dev, NVML_CLOCK_SM, &sm_clk_mhz))
+    if (nv_status == NVML_SUCCESS)
+      nvml_submit_gauge(dev_name, "frequency", "sm", 1e6 * sm_clk_mhz);


"sm" strikes me as an acronym – could you expand it for clarity?

octo · 2018-10-25T07:55:08Z

src/gpu_nvml.c

+    unsigned int mem_clk_mhz;
+    TRYOPT(nvmlDeviceGetClockInfo(dev, NVML_CLOCK_MEM, &mem_clk_mhz))
+    if (nv_status == NVML_SUCCESS)
+      nvml_submit_gauge(dev_name, "frequency", "mem", 1e6 * mem_clk_mhz);


Dito: "memory" is clearer than "mem".

octo

LGTM, thank you @qdbp!

qdbp added 4 commits September 10, 2018 10:33

add nvml module

654c9be

collect more stats

06bb233

fix formatting

179cf85

config: fix OB1, describe config errors

888928d

octo added the New plugin label Oct 4, 2018

octo requested changes Oct 21, 2018

View reviewed changes

octo changed the title ~~NEW PLUGIN: gpu_nvml, collect NVIDIA GPU stats.~~ New plugin: gpu_nvml collects NVIDIA GPU stats. Oct 22, 2018

collectd-bot added this to the Features milestone Oct 22, 2018

qdbp added 2 commits October 22, 2018 10:41

changes from PR review

912fe86

merge upstream

1c6fdba

rpv-tomsk reviewed Oct 23, 2018

View reviewed changes

qdbp added 2 commits October 23, 2018 20:00

address PR review issues

df275e0

fix formatting

82bbc79

octo reviewed Oct 25, 2018

View reviewed changes

rename plugin to "gpu_nvidia"

504ebae

octo approved these changes Oct 27, 2018

View reviewed changes

octo merged commit 10d9557 into collectd:master Oct 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New plugin: gpu_nvml collects NVIDIA GPU stats. #2923

New plugin: gpu_nvml collects NVIDIA GPU stats. #2923

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		for (int ix = 0; ix < device_count; ix++) {

		int is_match = ((1 << ix) & conf_match_mask) \|\| (conf_match_mask == 0);


		static int nvml_config(const char key, const char value) {

		char *eptr;

New plugin: gpu_nvml collects NVIDIA GPU stats. #2923

New plugin: gpu_nvml collects NVIDIA GPU stats. #2923

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!