Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/rdc/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ Full documentation for RDC is available at [ROCm DataCenter Tool User Guide](htt

- Stopping `rdcd`, or embedded RDC with `rdc_stop_embedded()`, no longer hangs. Rarely, the stop never returned and the process had to be killed.

- `rdcd` no longer crashes when the watches due in one update read more than 8,192 GPU-field pairs from AMD SMI, for example two `rdci dmon` sessions of 128 fields on 64 GPUs each.

- `rdcd` no longer crashes listing more than 128 (`RDC_MAX_NUM_DEVICES`) GPUs and GPU partitions; `rdc_device_get_all()` lists the first 128.

## RDC 1.3.1 for ROCm 7.14.0

### Added
Expand Down
6 changes: 4 additions & 2 deletions projects/rdc/include/rdc/rdc.h
Original file line number Diff line number Diff line change
Expand Up @@ -1165,9 +1165,11 @@ rdc_status_t rdc_field_update_all(rdc_handle_t p_rdc_handle, uint32_t wait_for_u
* @param[in] p_rdc_handle The RDC handler.
*
* @param[out] gpu_index_list Array reference to fill GPU indexes present on
* the system.
* the system. At most ::RDC_MAX_NUM_DEVICES GPUs and GPU partitions are
* listed; any beyond that are not.
*
* @param[out] count Number of GPUs returned in gpu_index_list.
* @param[out] count Number of GPUs returned in gpu_index_list, at most
* ::RDC_MAX_NUM_DEVICES.
*
* @retval ::RDC_ST_OK is returned upon successful call.
*/
Expand Down
2 changes: 1 addition & 1 deletion projects/rdc/include/rdc_lib/impl/RdcTelemetryModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class RdcTelemetryModule : public RdcTelemetry {
void get_fields_for_module(
rdc_gpu_field_t* fields, uint32_t fields_count,
std::map<RdcTelemetryPtr, std::vector<rdc_gpu_field_t>>& fields_in_module,
std::vector<rdc_gpu_field_value_t>& unsupport_fields); // NOLINT
std::vector<rdc_gpu_field_value_t>& unsupport_fields) const; // NOLINT
std::list<RdcTelemetryPtr> telemetry_modules_;
std::map<uint32_t, RdcTelemetryPtr> fields_id_module_;
};
Expand Down
16 changes: 15 additions & 1 deletion projects/rdc/rdc_libs/rdc/src/RdcEmbeddedHandler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ THE SOFTWARE.

#include <string.h>

#include <algorithm>
#include <atomic>

#include "amd_smi/amdsmi.h"
#include "common/rdc_fields_supported.h"
#include "rdc/rdc.h"
Expand Down Expand Up @@ -198,8 +201,19 @@ rdc_status_t RdcEmbeddedHandler::rdc_device_get_all(uint32_t gpu_index_list[RDC_
return status;
}

// gpu_index_list holds RDC_MAX_NUM_DEVICES entries.
int64_t total = std::max<int64_t>(device_count.value.l_int, 0);
if (total > RDC_MAX_NUM_DEVICES) {
static std::atomic<bool> reported{false};
if (!reported.exchange(true)) {
RDC_LOG(RDC_ERROR, "RDC lists " << RDC_MAX_NUM_DEVICES << " of the " << total
<< " GPUs and GPU partitions");
}
total = RDC_MAX_NUM_DEVICES;
}

// Assign the index to the index list
*count = device_count.value.l_int;
*count = static_cast<uint32_t>(total);
for (uint32_t i = 0; i < *count; i++) {
gpu_index_list[i] = i;
}
Expand Down
28 changes: 18 additions & 10 deletions projects/rdc/rdc_libs/rdc/src/RdcTelemetryModule.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ THE SOFTWARE.
*/
#include "rdc_lib/impl/RdcTelemetryModule.h"

#include <algorithm>
#include <memory>

#include "rdc_lib/RdcException.h"
Expand Down Expand Up @@ -108,11 +109,13 @@ RdcTelemetryModule::RdcTelemetryModule(std::list<RdcTelemetryPtr> telemetry_modu
void RdcTelemetryModule::get_fields_for_module(
rdc_gpu_field_t* fields, uint32_t fields_count,
std::map<RdcTelemetryPtr, std::vector<rdc_gpu_field_t>>& fields_in_module,
std::vector<rdc_gpu_field_value_t>& unsupport_fields) {
std::vector<rdc_gpu_field_value_t>& unsupport_fields) const {
for (uint32_t findex = 0; findex < fields_count; findex++) {
RdcTelemetryPtr module = fields_id_module_[fields[findex].field_id];
if (module) {
fields_in_module[module].push_back(fields[findex]);
// find(), not operator[], which would add an entry for every unknown field: only the
// constructor fills the map.
auto found = fields_id_module_.find(fields[findex].field_id);
if (found != fields_id_module_.end()) {
fields_in_module[found->second].push_back(fields[findex]);
} else {
RDC_LOG(RDC_DEBUG, "Unsupported field " << field_id_string(fields[findex].field_id));
rdc_gpu_field_value_t value;
Expand All @@ -137,15 +140,20 @@ rdc_status_t RdcTelemetryModule::rdc_telemetry_fields_value_get(rdc_gpu_field_t*
std::vector<rdc_gpu_field_value_t> unsupport_fields;
get_fields_for_module(fields, fields_count, fields_to_fetch, unsupport_fields);

auto ite = fields_to_fetch.begin();
for (; ite != fields_to_fetch.end(); ite++) {
rdc_gpu_field_t f[MAX_NUM_FIELDS];
std::copy(ite->second.begin(), ite->second.end(), f);
ite->first->rdc_telemetry_fields_value_get(f, ite->second.size(), callback, user_data);
for (auto& [module, due] : fields_to_fetch) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this better than o(n^2)?

// A library takes at most MAX_NUM_FIELDS fields per call, but the watches due in one
// update can hold more.
for (size_t first = 0; first < due.size(); first += MAX_NUM_FIELDS) {
const size_t n = std::min<size_t>(MAX_NUM_FIELDS, due.size() - first);
module->rdc_telemetry_fields_value_get(&due[first], static_cast<uint32_t>(n), callback,
user_data);
}
}

// Notify the caller unsupported fields
callback(&unsupport_fields[0], unsupport_fields.size(), user_data);
if (!unsupport_fields.empty()) {
callback(unsupport_fields.data(), static_cast<uint32_t>(unsupport_fields.size()), user_data);
}

return RDC_ST_OK;
}
Expand Down
108 changes: 108 additions & 0 deletions projects/rdc/tests/ci/test_fake_gpu_telemetry.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
"""How many GPUs and fields RDC handles at once, against the fake amd-smi.

Runs like test_fake_gpu.py (same environment variables and helpers). Also runs
rdc_telemetry_tests, the gtest of RdcTelemetryModule, from the tested prefix.
"""

import os
import re
import subprocess
import threading

import pytest

# The helpers and the `tools` fixture come from test_fake_gpu.py, not conftest.py:
# tests/ci/conftest.py is not installed with the suite (share/rdc/rdctst_tests/fake_gpu).
from test_fake_gpu import FACTOR, PREFIX, REQUIRE, Rdcd, _created_id, tools # noqa: F401

RDC_MAX_NUM_DEVICES = 128 # include/rdc/rdc.h
# 128 fields that RDC reads from amd-smi (rdci dmon -l), so that with 64 GPUs each watch
# hands 8,192 GPU-field pairs to the same library. Fields of another library would split them.
FIELDS_128 = (
list(range(1, 11))
+ list(range(100, 107))
+ [200, 201, 202, 300, 301]
+ list(range(400, 410))
+ list(range(500, 522))
+ [550]
+ list(range(600, 660))
+ [680]
+ list(range(700, 712))
)
assert len(FIELDS_128) == 128


def test_telemetry_module_gtest():
binary = os.path.join(PREFIX, "share", "rdc", "rdctst_tests", "rdc_telemetry_tests")
if not os.path.isfile(binary):
if REQUIRE:
pytest.fail(f"{binary} is missing")
pytest.skip(f"{binary} is missing")
r = subprocess.run([binary], capture_output=True, text=True, timeout=120 * FACTOR)
assert r.returncode == 0, (
f"rdc_telemetry_tests exited with {r.returncode}\n{r.stdout[-3000:]}{r.stderr[-2000:]}"
)


def test_more_gpus_and_partitions_than_rdc_lists(tools, tmp_path):
# 256 GPU partitions; RDC lists RDC_MAX_NUM_DEVICES of them. rdcd wrote all 256 into
# an array of 128 for discovery and for a job's start.
d = Rdcd(tools, tmp_path, gpus=128, RDC_FAKE_PARTITIONS=2).start()
try:
out = d.ok("discovery", "-l")
assert f"{RDC_MAX_NUM_DEVICES} GPUs found" in out, out
# Logged by the handler, or first by a GPU table that keeps only the first
# RDC_MAX_NUM_DEVICES, so that the handler never sees more.
logged = (
f"RDC lists {RDC_MAX_NUM_DEVICES} of the 256 GPUs and GPU partitions",
f"AMD SMI lists more than {RDC_MAX_NUM_DEVICES} GPUs; RDC uses the first of them",
)
assert any(line in d.output() for line in logged), d.output()
listed = re.findall(r"^(\d+)\s+Fake ", out, re.M)
assert listed == [str(i) for i in range(RDC_MAX_NUM_DEVICES)], out
gid = _created_id(d.ok("group", "-c", "big_job"))
d.ok("group", "-g", gid, "-a", "0,1")
d.ok("stats", "-s", "big_job_1", "-g", gid)
d.ok("stats", "-x", "big_job_1")
finally:
code = d.stop()
assert code == 0, f"rdcd exited with {code}\n{d.output()}"


def test_two_watches_of_8192_fields(tools, tmp_path):
# Both watches due in one update hand 16,384 fields to the amd-smi library at once.
# rdcd crashed in some runs, when the two came due together.
d = Rdcd(tools, tmp_path, gpus=128).start()
try:
groups = []
for first in (0, 64):
gid = _created_id(d.ok("group", "-c", f"half_{first}"))
d.ok("group", "-g", gid, "-a", ",".join(str(g) for g in range(first, first + 64)))
groups.append(gid)
fields = ",".join(str(f) for f in FIELDS_128)
fgid = _created_id(d.ok("fieldgroup", "-c", "fields_128", "-f", fields))

results = {}
start = threading.Barrier(len(groups), timeout=60 * FACTOR)

def dmon(gid):
try:
start.wait()
results[gid] = d.rdci("dmon", "-g", gid, "-f", fgid, "-c", "8", "-d", "100")
except BaseException as e: # noqa: BLE001 - reported below, in the test's thread
results[gid] = e

threads = [threading.Thread(target=dmon, args=(gid,)) for gid in groups]
for t in threads:
t.start()
for t in threads:
t.join()
for gid in groups:
r = results[gid]
if isinstance(r, BaseException):
raise r
assert r.returncode == 0, f"dmon on group {gid}: {r.returncode}\n{r.stdout}{r.stderr}"
d.check_alive("two dmon watches")
finally:
code = d.stop()
assert code == 0, f"rdcd exited with {code}\n{d.output()}"
24 changes: 23 additions & 1 deletion projects/rdc/tests/unit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,23 @@ install(
COMPONENT ${TESTS_COMPONENT}
)

# rdc_telemetry_tests: how RdcTelemetryModule hands watched fields to the telemetry
# libraries, checked with a stub library. Builds the module from source instead of
# linking librdc, whose load-time amd-smi initialization needs a GPU, so it runs on
# GPU-less CI.
add_executable(
rdc_telemetry_tests
${SRC_DIR}/test_telemetry_module.cc
${PROJECT_SOURCE_DIR}/rdc_libs/rdc/src/RdcTelemetryModule.cc
)
target_include_directories(
rdc_telemetry_tests
PRIVATE ${PROJECT_SOURCE_DIR}/include ${AMD_SMI_INCLUDE_DIR}
)
target_link_libraries(rdc_telemetry_tests PRIVATE rdc_bootstrap GTest::gtest_main pthread)
set_target_properties(rdc_telemetry_tests PROPERTIES INSTALL_RPATH "\$ORIGIN:\$ORIGIN/../../../lib")
add_test(NAME rdc_telemetry_tests COMMAND rdc_telemetry_tests)

# rdc_unit_tests: hardware-aware SmiUtils tests. Links librdc (flat table,
# partition helpers) and amd_smi. Provides its own main() to init amdsmi once.
# Hardware-dependent tests self-skip when no AMD GPU is present.
Expand All @@ -201,7 +218,12 @@ set_target_properties(
add_test(NAME ${RDC_UNIT_TESTS} COMMAND ${RDC_UNIT_TESTS})

install(
TARGETS ${RDC_CODEC_TESTS} ${RDC_UNIT_TESTS} rdc_diag_kernel_tests rdc_topology_tests
TARGETS
${RDC_CODEC_TESTS}
${RDC_UNIT_TESTS}
rdc_diag_kernel_tests
rdc_topology_tests
rdc_telemetry_tests
DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/${RDC}/rdctst_tests
COMPONENT ${TESTS_COMPONENT}
)
Expand Down
Loading
Loading