Add an amdgpu pmda #1975

fberat · 2024-05-03T12:19:49Z

This pmda retrieves data using the libdrm and libdrm-amdgpu libraries. It only retrieves general information, no process specific data.

Data retrieved includes memory usage, memory speed, GPU speed, temperature, etc ...

Old Radeon (Pre GCN 1.1) aren't supported.

src/pmdas/amdgpu/GNUmakefile

src/pmdas/amdgpu/help

natoscott

Looking good Fred!

Bunch of minor things described in individual comments. Beyond that, here's a list of other files that will need changes too:

configure.ac (if libdrm is absent, we need to switch this PMDA off in the build - I see there'a a libdrm.pc so I recommend using PKG_CHECK_MODULES like e.g. libsasl)
src/include/builddefs.in (makefile macro(s) based on configure.ac mechanism)
build/rpm/.spec (we'll need a sub-package for this new PMDA)
qa/xxxx[.out] (regression test or two, see the apache PMDA test qa/755 as example)
qa/admin/package-lists (list of rpm/deb packages that CI needs to install in order to build/test/release this).

If anything unclear (for sure some will be) - let's chat on slack.

natoscott · 2024-05-06T05:40:19Z

man/man1/pmdaamdgpu.1

+.\"
+.TH PMDAAMDGPU 1 "PCP" "Performance Co-Pilot"
+.SH NAME
+\f3pmdaamdgpu\f1 \- amdgpu gpu metrics domain agent (PMDA)


-> "AMD GPU metrics domain agent"

man/man1/pmdaamdgpu.1

natoscott · 2024-05-06T05:43:04Z

man/man1/pmdaamdgpu.1

+.PP
+The
+.B amdgpu
+PMDA exports metrics that measure gpu activity, memory utilization,


natoscott · 2024-05-06T06:04:31Z

man/man1/pmdaamdgpu.1

+.fi
+.ft 1
+.PP
+If you want to establish access to the names, help text and values for the amdgpu


amdgpu -> AMD GPU

natoscott · 2024-05-06T06:06:22Z

src/pmdas/amdgpu/GNUmakefile

+CFILES	= localdrm.c amdgpu.c
+HFILES	= localdrm.h
+DFILES	= README
+LLDLIBS	= $(PCP_PMDALIB) $(LIB_FOR_DLOPEN) -ldrm -ldrm_amdgpu


Depending on how the configure.ac machinery ends up (discussed elsewhere), the libraries listed here will likely end up accessed as makefile macros.

natoscott · 2024-05-06T06:31:29Z

src/pmdas/amdgpu/localdrm.c

+  int dev_count = drmGetDevices(NULL, 0);
+
+  if (dev_count <= 0) {
+      printf("No devices\n");


The diagnostic printf calls in this file should all become __pmNotifyErr calls which interacts with the logging subsystem a bit more nicely (with timestamp prefixes, etc) - and ensures we write into the log and not stdout (which may even have pmcd on the other end of it waiting for a PDU).

Oops, these are development artefacts, I'll rework/cleanup.

natoscott · 2024-05-06T06:34:31Z

src/pmdas/amdgpu/localdrm.c

+      memcpy(&p[amdgpu_count++], &temp[i], sizeof(drmDevicePtr));
+
+      /* Done with version */
+      drmFreeVersion(ver);


Do we need to close the fd (local variable on-stack)? Looks like it leaks an fd for each device here otherwise.

natoscott · 2024-05-06T06:41:37Z

src/pmdas/amdgpu/localdrm.c

+
+  return DRM_SUCCESS;
+}
+


There's alot of opening and closing of device files here, with the way these interfaces are setup (I think this is a bit based on nvidia again, and thats maybe made it more complex than necessary).

Going back to the earlier comment about need_refresh - currently we call all of these APIs on every fetch, for every GPU. If we are going to do that, we can collapse all of these library calls into a single function that opens and closes each GPU fd a maximum of once per fetch, and avoid system time overheads associated with doing this "for every GPU for every metric" (i.e. ~10x less work if we have 10 metrics).

natoscott · 2024-05-06T06:48:44Z

src/pmdas/amdgpu/pmns

+    mem_clock_max		AMDGPU:0:8
+    gpu_clock			AMDGPU:0:9
+    gpu_clock_max		AMDGPU:0:10
+    temperature			AMDGPU:0:11


With several of the above metrics, consider a "memory" subtree in the PMNS, e.g. amdgpu.memory.clock and so on (total, free, etc). Possibly same for amdgpu.gpu.clock and clock_max.

natoscott · 2024-05-06T06:48:46Z

src/pmdas/amdgpu/pmns

+    gpu_clock_max		AMDGPU:0:10
+    temperature			AMDGPU:0:11
+    load			AMDGPU:0:12
+    avg_pwr			AMDGPU:0:13


I'd be inclined to expand avg_pwr to 'average_power' here.

fberat · 2024-05-28T11:42:20Z

I believe I addressed most of the review finidings, I'll need to go through the code at least one more time though.

natoscott

Thanks Fred, good updates here. Re that QA test we talked about, check out:

"cd qa && ./new" to create a new test
qa/755 for a simple example (Apache PMDA)

natoscott · 2024-06-05T03:37:56Z

build/rpm/pcp.spec.in

+#
+%package pmda-amdgpu
+License: GPL-2.0-or-later
+Summary: Performance Co-Pilot (PCP) metrics from eBPF ELF modules


Should be more like AMD GPUs and less like eBPF ELF modules ;)

natoscott · 2024-06-05T03:38:56Z

build/rpm/redhat.spec

+#
+%package pmda-amdgpu
+License: GPL-2.0-or-later
+Summary: Performance Co-Pilot (PCP) metrics from eBPF ELF modules


Likewise here.

natoscott · 2024-06-05T03:41:23Z

build/rpm/redhat.spec

@@ -2269,6 +2272,23 @@ collecting metrics about web server logs.
 # end pcp-pmda-weblog
 # end C pmdas

+%if !%{disable_amdgpu}


This macro (disable_amdgpu) isn't defined AFAICT. Not 100% but I'm guessing it will be similar to the disable_resctrl definition which makes that package x86_64 only.

natoscott · 2024-06-05T03:45:16Z

build/rpm/redhat.spec

+This package contains the PCP Performance Metrics Domain Agent (PMDA) for
+extracting performance metrics from AMDGPU devices.
+# end pcp-pmda-amdgpu
+%endif


There's several other things that need to be done in the spec files to create a sub-package (e.g. a %files section). Simplest way to find 'em is to look at a similar sub-package - the closest to this new one may be pcp-pmda-resctrl (search on "resctrl" occurrences in each spec and mimic each section).

natoscott · 2024-06-05T03:55:11Z

config.guess

@@ -1,12 +1,14 @@
-#! /bin/sh
+#!/usr/bin/sh


Ah, thanks for updating these files Fred, well overdue. Is /usr/bin/sh guaranteed to exist on all platforms though? We tend to use /bin/sh everywhere else anyway.

That actually came with the configure update.
In theory nowadays everything is in /usr. There is a trend to remove /bin and /sbin.

@fberat yeah, I understand that's the trend (on Linux). The problem will come in on platforms that don't have such a trend, e.g. Mac OS ...

(base) nathans-mac:~ nathans$ uname -a Darwin nathans-mac 23.5.0 Darwin Kernel Version 23.5.0: Wed May 1 20:12:58 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6000 arm64 (base) nathans-mac:~ nathans$ ls -l /usr/bin/sh ls: /usr/bin/sh: No such file or directory (base) nathans-mac:~ nathans$

Ok, that's annoying, I may need to check with upstream config.{sub,guess} repo.

I'll investigate further, there is something odd, the file on my system doesn't match the one in the redhat-rpm-config repository.

Ok, found the reason, it looks like there is a systemic modification of shebang in fedora when rpm are built. I'll revert the shebang back to the original value on my next update.

natoscott · 2024-06-05T04:10:34Z

src/pmdas/amdgpu/amdgpu.c

+    case AMDGPU_TEMPERATURE:
+      if (pcp_amdgpuinfo.info[inst].failed[AMDGPU_TEMPERATURE])
+        return PM_ERR_VALUE;
+      /* In millidegrees Celsius */


I see the label callbacks now but should we add "units":"millidegrees celcius" as a label for this one?

natoscott · 2024-06-05T04:11:23Z

src/pmdas/amdgpu/amdgpu.c

+    if (autorefresh > 0) {
+      autorefresh = 0;
+      for (int i = 0;i < AMDGPU_REFRESHER_COUNT;i++) {
+	  pmNotifyErr(LOG_ERR, "Refreshing %d", i);


Too verbose by default here, I think this is going to end up in a log file once every second?

I need to cleanup, at the end of the day, you were faster than me to review these changes :)

natoscott · 2024-06-05T04:13:12Z

src/pmdas/amdgpu/drm.c

+
+#ifndef DSOSUFFIX
+#define DSOSUFFIX "so"
+#endif


Pretty sure this agent is Linux-only, so safe to hard-code this if you like.

natoscott · 2024-06-05T04:14:32Z

src/pmdas/amdgpu/drm.c

+
+      if (strcmp(ver->name, "amdgpu")) {
+	  drmFreeVersion(ver);
+	  continue;


I think we may leak an open fd here?

natoscott · 2024-06-05T04:15:43Z

src/pmdas/amdgpu/drm.c

+
+      /* Done with version */
+      drmFreeVersion(ver);
+  }


And also here at the last part of the loop? Could close it unconditionally right after drmGetVersion perhaps.

fberat · 2024-06-05T12:24:19Z

src/pmdas/amdgpu/amdgpu.c

+    switch (item) {
+    case AMDGPU_MEMORY_USED:
+      atom->ull = pcp_amdgpuinfo.info[inst].memory.used;
+      pmNotifyErr(LOG_ERR, "Getting used memory %lu", atom->ull);


natoscott · 2024-07-04T07:14:24Z

Last handful of small things @fberat ...

amdgpu_fetch()

     if (refresher_list[cluster][item] == NULL)
       continue;

We have a check that 'cluster' is within range, does 'item' need
a similar check to prevent possible out-of-bounds access? (i.e.
via a maliciously crafted PDU with pmID item >= MAX_ITEM_COUNT).

setup_gcard_indom()

pmNotifyErr(LOG_WARNING, "setup_gcard_indom: got %d cards" ...

INFO level might be more appropriate for this one?

pmNotifyErr(LOG_ERR, "Refreshed memory (%lx)", memory.used);

-> leftover temp diagnostic? (or add pmDebugOptions.appl2 guard)

PMDA README file has the word Readme at the start - intentional?
Just looks a bit odd. The heading underline could use one more
equals character as well. :)

qa/1772 has template comment still. [who are you?] -> Red Hat or
Fred (yourself). Also need to change this line, e.g.:

# test for-some-thing || _notrun No support for some-thing

to

test -d $PCP_PMDAS_DIR/amdgpu || _notrun No support for AMD GPU metrics

fberat · 2024-07-04T07:29:07Z

Last handful of small things @fberat ...

amdgpu_fetch()
     if (refresher_list[cluster][item] == NULL)
       continue;
We have a check that 'cluster' is within range, does 'item' need a similar check to prevent possible out-of-bounds access? (i.e. via a maliciously crafted PDU with pmID item >= MAX_ITEM_COUNT).

Agreed.

setup_gcard_indom()
pmNotifyErr(LOG_WARNING, "setup_gcard_indom: got %d cards" ...
INFO level might be more appropriate for this one?

Agreed.

pmNotifyErr(LOG_ERR, "Refreshed memory (%lx)", memory.used);
-> leftover temp diagnostic? (or add pmDebugOptions.appl2 guard)

Yes, removing.

PMDA README file has the word Readme at the start - intentional? Just looks a bit odd. The heading underline could use one more equals character as well. :)

Likely not intentional, removing. Equal character added.

qa/1772 has template comment still. [who are you?] -> Red Hat or Fred (yourself). Also need to change this line, e.g.:

Red Hat added.

# test for-some-thing || _notrun No support for some-thing

to

test -d $PCP_PMDAS_DIR/amdgpu || _notrun No support for AMD GPU metrics

Done.

Thanks for the review, I'll push the update.

This pmda retrieves data using the libdrm and libdrm-amdgpu libraries. It only retrieves general information, no process specific data. Data retrieved includes memory usage, memory speed, GPU speed, temperature, etc ... Old Radeon (Pre GCN 1.1) aren't supported. Signed-off-by: Frédéric Bérat <[email protected]>

fberat commented May 3, 2024

View reviewed changes

src/pmdas/amdgpu/GNUmakefile Outdated Show resolved Hide resolved

fberat commented May 3, 2024

View reviewed changes

src/pmdas/amdgpu/help Outdated Show resolved Hide resolved

natoscott reviewed May 6, 2024

View reviewed changes

fberat force-pushed the devel/amdgpu branch from ef7ae4d to 3009f1b Compare May 28, 2024 11:40

natoscott reviewed Jun 5, 2024

View reviewed changes

fberat commented Jun 5, 2024

View reviewed changes

fberat force-pushed the devel/amdgpu branch 2 times, most recently from 89152be to 8e6c36e Compare June 19, 2024 13:57

fberat force-pushed the devel/amdgpu branch from 8e6c36e to 2f09707 Compare July 4, 2024 07:30

natoscott merged commit e92fe2e into performancecopilot:main Jul 5, 2024
17 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an amdgpu pmda #1975

Add an amdgpu pmda #1975

fberat commented May 3, 2024

natoscott left a comment

natoscott May 6, 2024

natoscott May 6, 2024

natoscott May 6, 2024

natoscott May 6, 2024

natoscott May 6, 2024

fberat May 6, 2024

natoscott May 6, 2024

natoscott May 6, 2024

natoscott May 6, 2024

natoscott May 6, 2024

fberat commented May 28, 2024

natoscott left a comment

natoscott Jun 5, 2024

natoscott Jun 5, 2024

natoscott Jun 5, 2024

natoscott Jun 5, 2024

natoscott Jun 5, 2024

fberat Jun 5, 2024

natoscott Jun 6, 2024

fberat Jun 6, 2024

fberat Jun 6, 2024

fberat Jun 6, 2024

natoscott Jun 5, 2024

natoscott Jun 5, 2024

fberat Jun 5, 2024

natoscott Jun 5, 2024

natoscott Jun 5, 2024

natoscott Jun 5, 2024

fberat Jun 5, 2024

natoscott commented Jul 4, 2024

fberat commented Jul 4, 2024

Add an amdgpu pmda #1975

Add an amdgpu pmda #1975

Conversation

fberat commented May 3, 2024

natoscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fberat commented May 28, 2024

natoscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natoscott commented Jul 4, 2024

fberat commented Jul 4, 2024