amd_smi: add gpu_metrics events, refine descriptions, and suppress sentinel-valued fields by djwoun · Pull Request #568 · icl-utk-edu/papi

djwoun · 2026-02-25T03:21:27Z

Pull Request Description

Expands metric coverage, updates descriptions for select existing GPU metrics events, suppresses registration of struct-backed events when AMD SMI returns sentinel values.

In particular, it:

adds and expands GPU metrics events
updates descriptions for select existing GPU metrics events where better AMD SMI documentation is available
filters out sentinel-backed fields at registration time for struct-based event groups so unsupported metrics are not exposed as garbage values such as -1

Author Checklist

Description
Why this PR exists. Reference all relevant information, including background, issues, test failures, etc
Commits
Commits are self contained and only do one thing
Commits have a header of the form: module: short description
Commits have a body (whenever relevant) containing a detailed description of the addressed problem and its solution
Tests
The PR needs to pass all the tests

Treece-Burgess · 2026-02-25T18:34:12Z

I am reviewing this PR.

Treece-Burgess · 2026-02-25T21:41:04Z

        if (add_event(&idx, name_buf, descr_buf, d, 10, 0, PAPI_MODE_READ,
                      access_amdsmi_gpu_metrics) != PAPI_OK)
          return PAPI_ENOMEM;
+


The new events all check out on Odyssey, but on Illyad with ROCm 7.1.1, the events:

amd_smi:::accumulation_counter amd_smi:::prochot_residency_acc amd_smi:::ppt_residency_acc amd_smi:::socket_thm_residency_acc amd_smi:::vr_thm_residency_acc amd_smi:::hbm_thm_residency_acc

will show a counter value of -1 when ran with papi_command_line. Do you know why this is occurring?

There is a small problem where amdsmi_get_gpu_metrics_info_p will return true if it returns just one true metric out of the 20 metrics it queries for.

I'm thinking maybe I should add additional checks to see if it returns sentinel values?

I think that would be a good idea! What were you thinking the check could be?

AMD SMI calls can return AMDSMI_STATUS_SUCCESS even when only part of the output struct is actually valid, and the unsupported fields are left at sentinel values. For the residency and accumulation counters that sentinel is UINT64_MAX, which PAPI ends up showing as -1. But the same pattern shows up in other struct-returning calls too, with UINT16_MAX, UINT32_MAX, UINT64_MAX, and in a few cases things like UINT8_MAX.

So the fix in this PR is the generic one: zero-init the struct, probe it in amds.c, and only register an event if the specific backing field is not its width-appropriate sentinel. That is why the PR applies the same check to other struct-based events.

I saw the use of conditionals checking UINT16_MAX, UINT32_MAX, UINT64_MAX, etc. A follow up question is: How do you know those are the correct? For example, you have the following conditional check:

if (pinfo.current_socket_power != UINT32_MAX && pinfo.current_socket_power != UINT16_MAX)

Looking at documentation current_socket_power only has UINT32_t. However, you also check UINT16_MAX.

I added the extra UINT16_MAX check because I was seeing some uint32_t AMD SMI fields come back as 65535. After printing the values from amdsmi_get_power_info_p(), that does appear to be what is happening for average_socket_power.

…ntinel-valued fields

Treece-Burgess · 2026-03-23T22:17:54Z

+    memset(&dummy_usage, 0, sizeof(dummy_usage));
+    if (amdsmi_get_gpu_activity_p && amdsmi_get_gpu_activity_p(device_handles[d], &dummy_usage) == AMDSMI_STATUS_SUCCESS) {
+      if (dummy_usage.gfx_activity != UINT32_MAX && dummy_usage.gfx_activity != UINT16_MAX) {
+        CHECK_EVENT_IDX(idx);


Was the CHECK_EVENT_IDX here and the two below originally forgotten when amd_smi was merged into master? From the diff it appears that way.

Yeah, looks like it was missing before.

Treece-Burgess · 2026-03-23T22:33:08Z

+        if (amdsmi_get_clock_info_p(device_handles[d], clk_types[t], &info) != AMDSMI_STATUS_SUCCESS)
          continue;
        for (int f = 0; f < 5; ++f) {
+          if (f == 4 && info.clk_deep_sleep == UINT8_MAX)


What is the reasoning for continuing here? Could you add a comment above the for loop header for documentation.

That continue is only there to skip adding the deep_sleep event when clk_deep_sleep is UINT8_MAX.

Treece-Burgess · 2026-03-24T14:21:33Z

        if (add_event(&idx, name_buf, descr_buf, d, 10, 0, PAPI_MODE_READ,
                      access_amdsmi_gpu_metrics) != PAPI_OK)
          return PAPI_ENOMEM;
+


I saw the use of conditionals checking UINT16_MAX, UINT32_MAX, UINT64_MAX, etc. A follow up question is: How do you know those are the correct? For example, you have the following conditional check:

if (pinfo.current_socket_power != UINT32_MAX && pinfo.current_socket_power != UINT16_MAX)

Looking at documentation current_socket_power only has UINT32_t. However, you also check UINT16_MAX.

Treece-Burgess · 2026-03-24T15:32:09Z

@@ -1279,128 +1153,121 @@ static int init_event_table(void) {
    // PCIe information events
    if (amdsmi_get_pcie_info_p) {


From my counting 36 new events are added. Of the 36, 7 events do not appear on either the MI210 or MI300A they are:

pcie_nak_sent_count_acc pcie_nak_rcvd_count_acc average_vclk1_frequency average_dclk1_frequency vcn_activity_vcn jpeg_activity_jpeg xcp_gfx_below_host_limit_acc_xcp

Do you know the architecture these events appear on?

I am not sure but I would guess some older generations of AMD gpus because this function was created with ROCm SMI.

Treece-Burgess · 2026-03-24T15:36:32Z

@@ -1279,128 +1153,121 @@ static int init_event_table(void) {
    // PCIe information events


When running ./papi_component_avail on the master branch, amd_smi shows 342 native events on Odyssey at Oregon with ROCm 7.2.0. Doing the same for this PR, I see 390 native events. An increase is expected, but from my count 71 new events show in the output of papi_native_avail. Meaning we should see 413 output instead of 390.

This PR adds new events, but it also adds registration-time sentinel filtering to some existing struct-based events. So the expected total is not just 342 + 71; it is 342 + added - filtered. On Odyssey that appears to be why the total is 390 instead of 413.

Treece-Burgess · 2026-03-24T15:52:52Z

+    break;
+  }
+  case 41: {
+    uint32_t xcp_index = event->subvariant >> 16;


Q: I am not familiar enough with the workflow, so what will event->subvariant hold? I know from one of the header files it is a uint32_t, but how is both right shifting and ANDing going to give us the correct indices?

subvariant is being used here to carry the indices needed for the XCP array lookups. These events need two indices, not one: the XCP index and the inner element index. Since native_event_t only has one subvariant field, both are packed into that uint32_t during registration and unpacked in the accessor.

djwoun requested review from Treece-Burgess, dbarry9 and tokey-tahmid February 25, 2026 03:21

Treece-Burgess requested changes Feb 25, 2026

View reviewed changes

djwoun changed the title ~~amd_smi: add gpu_metrics accumulation and and residency counters~~ amd_smi: add gpu_metrics events, refine descriptions, and suppress sentinel-valued fields Mar 19, 2026

djwoun force-pushed the amd-smi-gpu-metrics-events branch from a28e459 to 6bc178d Compare March 19, 2026 21:59

amd_smi: add gpu_metrics events, refine descriptions, and suppress se…

f78936b

…ntinel-valued fields

djwoun force-pushed the amd-smi-gpu-metrics-events branch from 83f2477 to f78936b Compare March 19, 2026 23:07

Treece-Burgess requested changes Mar 24, 2026

View reviewed changes

		@@ -1279,128 +1153,121 @@ static int init_event_table(void) {
		// PCIe information events
		if (amdsmi_get_pcie_info_p) {

Conversation

djwoun commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

Author Checklist

Uh oh!

Treece-Burgess commented Feb 25, 2026

Uh oh!

Uh oh!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

djwoun commented Feb 25, 2026 •

edited

Loading