Skip to content
This repository has been archived by the owner on Jan 26, 2024. It is now read-only.

Commit

Permalink
SWDEV-351980 - Fix a race condition when setting the callbacks
Browse files Browse the repository at this point in the history
When acquiring the reader's lock (sem_sync()...sem_release()), it is
possible for a writer to squeeze by if the reader goes into sync_wait().
The writer could re-acquire the entry.sync between entry.sem > 0 and
entry.sync = 0.

void sync_wait(uint32_t id) {
    sem_decrement(id);
    while (entry(id).sync.load()) {}
    // <--- HERE
    sem_increment(id);
}

This could result in both the reader and the writer accessing
{ callback, arg } at the same time, and the reader could read
inconsistent data, for example: { new callback, old arg }.

The solution is to re-test entry.sync when returning from sync_wait():

void sem_sync(uint32_t id) {
    sem_increment(id);
-   if (entry(id).sync.load() == true) sync_wait(id);
+   while (entry(id).sync.load() == true) sync_wait(id);
}

(cherry picked from commit 7e9b355)

SWDEV-351980 - Use std::shared_mutex

Replace the custom reader/writer lock with the standard implementation
available in C++17.

(cherry picked from commit 7526b42)

Improve hip_prof_api.h's readability

- Don't pass uint32_t arguments by reference.
- Use nullptr instead of NULL.
- Don't add frivolous typedefs.
- Use correct types when available instead of generic integral types.
- Make all roctracer callbacks extern "C" to prepare for a future change
  that will be removing their declaration from hip_runtime_api.h.
- Rename cb/sem sync and release functions -> reader_lock/writer_lock
  acquire and release.

(cherry picked from commit 5a6a83e)

SWDEV-351980 - Move hip_api_data and record to the HIP function's stack

Since the hip_api_data and record are only needed at the HIP function's
scope, there is no need to allocate/free them in the ROCtracer activity
callback, they can reside on the HIP function's stack frame.

This solves an issue with the thread local stacks of records the tracer
maintains that are destroyed first (before any global destructor) on
process exit, making it impossible to use HIP functions in global
destructors when the profiler is enabled.

(cherry picked from commit 5ea0e6b)

SWDEV-351980 - Remove IS_PROFILER_ON

The CallbacksTable::is_enabled() can simply be implemented by checking
if enabled_api_count is > 0. The ROCclr does not use IS_PROFILER_ON
to report asynchronous activities.

(cherry picked from commit dae2ea8)

SWDEV-351980 - Consolidate registration tables in the roctracer library

Remove the api_callbacks_table_t that was holding the API activities and
user callbacks. Instead use a single roctracer callback (TracerCallback)
used to report both API activities and callbacks.

Remove the hipInitActivityCallback that was setting the ROCtracer
callback and memory pool for asynchronous activities as it did not
allow disctinct pools to be used for each activity.  Instead, use
hipRegisterTracerCallback to set the single roctracer callback.

(cherry picked from commit b9cf518)

SWDEV-351980 - Remove the ROCtracer private interface from the public header

(cherry picked from commit 5d71ec1)

SWDEV-359838 - Add a phase data pointer to the hip_api_data_t

To avoid using the thread local std::stack to remember the phase enter
timestamp, the tracer tool uses the phase data to store the timestamp.

(cherry picked from commit 928684d)

Fix a build error when compiling with clang

Fix the following error:

hip_intercept.cpp:52:7: error: reinterpret_cast from 'const void *' to 'decltype(activity_prof::report_activity.load())' (aka 'int (*)(activity_domain_t, unsigned int, void *)') casts away qualifiers
      reinterpret_cast<decltype(activity_prof::report_activity.load())>(function),
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

by replacing the 'const void *function' argument with the correct type.

(cherry picked from commit be33ec5)
Change-Id: I0fd0121ea58eb8f6aa3f6511303d3f2baa1191ad
  • Loading branch information
lmoriche committed Oct 13, 2022
1 parent 25a8ae2 commit ebe3ab4
Show file tree
Hide file tree
Showing 10 changed files with 62 additions and 274 deletions.
1 change: 1 addition & 0 deletions include/hip/amd_detail/hip_prof_str.h
Original file line number Diff line number Diff line change
Expand Up @@ -3124,6 +3124,7 @@ typedef struct hip_api_data_s {
hipStream_t stream;
} hipWaitExternalSemaphoresAsync;
} args;
uint64_t *phase_data;
} hip_api_data_t;

// HIP API callbacks args data filling macros
Expand Down
7 changes: 1 addition & 6 deletions src/amdhip.def
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,7 @@ hipConfigureCall
hipSetupArgument
hipLaunchByPtr
hipLaunchKernel
hipRegisterApiCallback
hipRemoveApiCallback
hipRegisterActivityCallback
hipRemoveActivityCallback
hipRegisterTracerCallback
hipApiName
hipKernelNameRef
hipBindTexture
Expand Down Expand Up @@ -254,8 +251,6 @@ hipProfilerStart
hipProfilerStop
hipCreateSurfaceObject
hipDestroySurfaceObject
hipInitActivityCallback
hipEnableActivityCallback
hipGetCmdName
hipMipmappedArrayCreate
hipMallocMipmappedArray
Expand Down
15 changes: 3 additions & 12 deletions src/hip_activity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,8 @@
THE SOFTWARE. */

#include "platform/activity.hpp"

extern "C" void hipInitActivityCallback(void* id_callback, void* op_callback, void* arg) {
activity_prof::CallbacksTable::init(reinterpret_cast<activity_prof::id_callback_fun_t>(id_callback),
reinterpret_cast<activity_prof::callback_fun_t>(op_callback),
arg);
}

extern "C" bool hipEnableActivityCallback(unsigned op, bool enable) {
return activity_prof::CallbacksTable::SetEnabled(op, enable);
}
#include <hip/hip_runtime_api.h>

extern "C" const char* hipGetCmdName(unsigned op) {
return getOclCommandKindString(static_cast<uint32_t>(op));
}
return getOclCommandKindString(static_cast<cl_command_type>(op));
}
4 changes: 1 addition & 3 deletions src/hip_graph_internal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -688,10 +688,8 @@ hipError_t hipGraphExec::CreateQueues(size_t numQueues) {
parallelQueues_.reserve(numQueues);
for (size_t i = 0; i < numQueues; i++) {
amd::HostQueue* queue;
cl_command_queue_properties properties =
(callbacks_table.is_enabled() || HIP_FORCE_QUEUE_PROFILING) ? CL_QUEUE_PROFILING_ENABLE : 0;
queue = new amd::HostQueue(*hip::getCurrentDevice()->asContext(),
*hip::getCurrentDevice()->devices()[0], properties,
*hip::getCurrentDevice()->devices()[0], 0,
amd::CommandQueue::RealTimeDisabled, amd::CommandQueue::Priority::Normal);

bool result = (queue != nullptr) ? queue->create() : false;
Expand Down
7 changes: 1 addition & 6 deletions src/hip_hcc.def.in
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,7 @@ hipConfigureCall
hipSetupArgument
hipLaunchByPtr
hipLaunchKernel
hipRegisterApiCallback
hipRemoveApiCallback
hipRegisterActivityCallback
hipRemoveActivityCallback
hipRegisterTracerCallback
hipApiName
hipKernelNameRef
hipBindTexture
Expand Down Expand Up @@ -254,8 +251,6 @@ hipProfilerStart
hipProfilerStop
hipCreateSurfaceObject
hipDestroySurfaceObject
hipInitActivityCallback
hipEnableActivityCallback
hipGetCmdName
hiprtcAddNameExpression
hiprtcCompileProgram
Expand Down
7 changes: 1 addition & 6 deletions src/hip_hcc.map.in
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,6 @@ global:
hipSetupArgument;
hipLaunchByPtr;
hipLaunchKernel;
hipRegisterApiCallback;
hipRemoveApiCallback;
hipRegisterActivityCallback;
hipRemoveActivityCallback;
hipApiName;
hipKernelNameRef;
hipKernelNameRefByPtr;
Expand Down Expand Up @@ -272,8 +268,6 @@ global:
hipDestroySurfaceObject*;
hipHccModuleLaunchKernel*;
hipExtModuleLaunchKernel*;
hipInitActivityCallback*;
hipEnableActivityCallback*;
hipGetCmdName*;
hipExtStreamCreateWithCUMask;
hipStreamGetPriority;
Expand Down Expand Up @@ -500,6 +494,7 @@ global:
hipUserObjectRetain;
hipGraphRetainUserObject;
hipGraphReleaseUserObject;
hipRegisterTracerCallback;
local:
*;
} hip_5.2;
43 changes: 13 additions & 30 deletions src/hip_intercept.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,50 +25,33 @@

// HIP API callback/activity

api_callbacks_table_t callbacks_table;

extern const std::string& FunctionName(const hipFunction_t f);

const char* hipKernelNameRef(const hipFunction_t f) { return FunctionName(f).c_str(); }
extern "C" {

int hipGetStreamDeviceId(hipStream_t stream) {
if (!hip::isValid(stream)) {
return -1;
}
hip::Stream* s = reinterpret_cast<hip::Stream*>(stream);
return (s != nullptr)? s->DeviceId() : ihipGetDevice();
}

const char* hipKernelNameRefByPtr(const void* hostFunction, hipStream_t) {
if (hostFunction == NULL) {
return NULL;
}
return PlatformState::instance().getStatFuncName(hostFunction);
return (s != nullptr) ? s->DeviceId() : ihipGetDevice();
}

hipError_t hipRegisterApiCallback(uint32_t id, void* fun, void* arg) {
return callbacks_table.set_callback(id, reinterpret_cast<api_callbacks_table_t::fun_t>(fun), arg) ?
hipSuccess : hipErrorInvalidValue;
const char* hipKernelNameRef(const hipFunction_t function) {
return FunctionName(function).c_str();
}

hipError_t hipRemoveApiCallback(uint32_t id) {
return callbacks_table.set_callback(id, NULL, NULL) ? hipSuccess : hipErrorInvalidValue;
const char* hipKernelNameRefByPtr(const void* host_function, hipStream_t stream) {
[](auto&&...) {}(stream);
return (host_function != nullptr) ? PlatformState::instance().getStatFuncName(host_function)
: nullptr;
}

hipError_t hipRegisterActivityCallback(uint32_t id, void* fun, void* arg) {
return callbacks_table.set_activity(id, reinterpret_cast<api_callbacks_table_t::act_t>(fun), arg) ?
hipSuccess : hipErrorInvalidValue;
void hipRegisterTracerCallback(int (*function)(activity_domain_t domain, uint32_t operation_id,
void* data)) {
activity_prof::report_activity.store(function, std::memory_order_relaxed);
}

hipError_t hipRemoveActivityCallback(uint32_t id) {
return callbacks_table.set_activity(id, NULL, NULL) ? hipSuccess : hipErrorInvalidValue;
}

hipError_t hipEnableTracing(bool enabled) {
callbacks_table.set_enabled(enabled);
return hipSuccess;
}
const char* hipApiName(uint32_t id) { return hip_api_name(id); }

const char* hipApiName(uint32_t id) {
return hip_api_name(id);
}
} // extern "C"
Loading

0 comments on commit ebe3ab4

Please sign in to comment.