Improve pool error checking

Check if a default pool is defined when enabling activities.

Set default pool to undefined if it is deleted.

Disable activities associated with the pool when it is deleted.

Document restrictions on deleting pools.

Change-Id: Ide466b55cab12ca2dd67d9f26b578f421e45a376


[ROCm/roctracer commit: feb652e45d]

projects/roctracer/inc/roctracer.h

@@ -60,7 +60,7 @@ extern "C" {
  * that uses this interface is only compatible with the installed library if
  * the major version numbers match and the interface minor version number is
  * less than or equal to the installed library minor version number.
- * 
+ *
  * @{
  */
 
@@ -388,13 +388,21 @@ static inline roctracer_status_t roctracer_next_record(
 /**
  * Memory pool allocator callback.
  *
- * \p ptr is the allocated memory when \p size is greater than 0 and the
- * address of the memory to deallocate when \p size is o.
+ * If \p *ptr is NULL, then allocate memory of \p size bytes and save address
+ * in \p *ptr.
+ * 
+ * If \p *ptr is non-NULL and size is non-0, then reallocate the memory at \p
+ * *ptr with size \p size and save the address in \p *ptr. The memory will have
+ * been allocated by the same callback.
+ * 
+ * If \p *ptr is non-NULL and size is 0, then deallocate the memory at \p *ptr.
+ * The memory will have been allocated by the same callback.
  *
- * \p size is the size of the allocation request in bytes if greater than 0.
- * If 0 requests deallocation.
+ * \p size is the size of the memory allocation or reallocation, or 0 if
+ * deallocating.
  *
- * \p arg Argument provided when the callback is defined.
+ * \p arg Argument provided in the ::roctracer_properties_t passed to the
+ * ::roctracer_open_pool function.
  */
 typedef void (*roctracer_allocator_t)(char** ptr,
                                       size_t size,
@@ -436,7 +444,7 @@ typedef struct {
 
   /**
    * The allocator function to use to allocate and deallocate the buffer. If
-   * NULL then \p malloc and \p free are used.
+   * NULL then \p malloc, \p realloc, and \p free are used.
    */
   roctracer_allocator_t alloc_fun;
 
@@ -464,9 +472,10 @@ typedef void roctracer_pool_t;
 /**
  * Create tracer memory pool.
  *
- * If \p pool is not NULL, returns the created memory pool.
+ * If \p pool is not NULL, returns the created memory pool. Does not change the
+ * default memory pool.
  *
- * If \p pool is NULL sets the default memory pool to the created pool if not
+ * If \p pool is NULL, sets the default memory pool to the created pool if not
  * already defined. Otherwise, return an error.
  *
  * @param[in] properties Tracer memory pool properties.
@@ -506,38 +515,49 @@ static inline roctracer_status_t roctracer_open_pool(
 /**
  * Close tracer memory pool.
  *
- * @param[in] pool Memory pool to close. If NULL, the default memory pool is closed and set to undefined.
+ * All enabled activities that use the pool must have completed writing to the
+ * pool, before deleting the pool. Deleting a pool automatically disables any
+ * activities that specify the pool, and flushes it.
+ *
+ * @param[in] pool Memory pool to close. If NULL, the default memory pool is
+ * closed if defined. The default memory pool is set to undefined if closed.
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
- * successfully.
+ * successfully or pool was NULL and there is no default pool.
  */
 roctracer_status_t roctracer_close_pool_expl(
     roctracer_pool_t* pool);
 
 /**
- * Close default tracer memory pool.
+ * Close default tracer memory pool, if defined, and set to undefined.
+ *
+ * All enabled activities that use the pool must have completed writing to the
+ * pool, before deleting the pool. Deleting a pool automatically disables any
+ * activities that specify the pool, and flushes it.
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
- * successfully.
+ * successfully or there is no default pool.
  */
 static inline roctracer_status_t roctracer_close_pool() { return roctracer_close_pool_expl(NULL); }
 
 /**
  * Query and set the default memory pool.
  *
- * @param[in] pool If not NULL, change the current default pool to \p pool.
+ * @param[in] pool If not NULL, change the current default pool to \p pool. If
+ * NULL, the default pool is not changed.
  *
- * @return Return the current default memory pool before any change.
+ * @return Return the current default memory pool before any change, or NULL if
+ * none is defined.
  */
 // Return current default pool
 // Set new default pool if the argument is not NULL
 roctracer_pool_t* roctracer_default_pool_expl(
-    roctracer_pool_t* pool);  // [in] new default pool if not NULL
+    roctracer_pool_t* pool);
 
 /**
- * Query current default memory pool.
+ * Query the current default memory pool.
  *
- * @return Return the current default memory pool.
+ * @return Return the current default memory pool, or NULL is none is defined.
  */
 static inline roctracer_pool_t* roctracer_default_pool() {
   return roctracer_default_pool_expl(NULL);
@@ -556,6 +576,8 @@ static inline roctracer_pool_t* roctracer_default_pool() {
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
  * successfully.
+ *
+ * @retval ROCTRACER_STATUS_ERROR \p pool is NULL and no default pool is defined.
  */
 roctracer_status_t roctracer_enable_op_activity_expl(
     activity_domain_t domain,
@@ -572,6 +594,8 @@ roctracer_status_t roctracer_enable_op_activity_expl(
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
  * successfully.
+ *
+ * @retval ROCTRACER_STATUS_ERROR No default pool is defined.
  */
 static inline roctracer_status_t roctracer_enable_op_activity(
     activity_domain_t domain,
@@ -591,6 +615,8 @@ static inline roctracer_status_t roctracer_enable_op_activity(
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
  * successfully.
+ *
+ * @retval ROCTRACER_STATUS_ERROR \p pool is NULL and no default pool is defined.
  */
 roctracer_status_t roctracer_enable_domain_activity_expl(
     activity_domain_t domain,  // tracing domain
@@ -604,6 +630,8 @@ roctracer_status_t roctracer_enable_domain_activity_expl(
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
  * successfully.
+ *
+ * @retval ROCTRACER_STATUS_ERROR No default pool is defined.
  */
 static inline roctracer_status_t roctracer_enable_domain_activity(
     activity_domain_t domain)
@@ -620,6 +648,8 @@ static inline roctracer_status_t roctracer_enable_domain_activity(
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
  * successfully.
+ *
+ * @retval ROCTRACER_STATUS_ERROR \p pool is NULL and no default pool is defined.
  */
 roctracer_status_t roctracer_enable_activity_expl(
     roctracer_pool_t* pool);
@@ -630,6 +660,8 @@ roctracer_status_t roctracer_enable_activity_expl(
  *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
  * successfully.
+ *
+ * @retval ROCTRACER_STATUS_ERROR No default pool is defined.
  */
 static inline roctracer_status_t roctracer_enable_activity() {
   return roctracer_enable_activity_expl(NULL);
@@ -672,6 +704,10 @@ roctracer_status_t roctracer_disable_activity();
 /**
  * Flush available activity records for a memory pool.
  *
+ * If flushing encounters an activity record still being written, flushing
+ * stops. Use a subsequent flush when the record has completed being written to
+ * resume the flush.
+ *
  * @param[in] pool The memory pool to flush. If NULL, flushes the default memory
  * pool.
  *
@@ -684,6 +720,10 @@ roctracer_status_t roctracer_flush_activity_expl(
 /**
  * Flush available activity records for the default memory pool.
  *
+ * If flushing encounters an activity record still being written, flushing
+ * stops. Use a subsequent flush when the record has completed being written to
+ * resume the flush.
+ *
  * @retval ::ROCTRACER_STATUS_SUCCESS The function has been executed
  * successfully.
  */

projects/roctracer/src/core/journal.h

@@ -31,11 +31,14 @@ namespace roctracer {
 
 template <typename Data> class Journal {
  public:
-  /* Insert { domain, op } into the journal. Return false if the insertion failed.  */
+  /* Insert { domain, op } into the journal. Return false if the insertion
+  updated an existing entry.  */
   template <typename T = Data, std::enable_if_t<std::is_constructible_v<Data, T>, int> = 0>
   bool Insert(roctracer_domain_t domain, uint32_t op, T&& data) {
     std::lock_guard lock(mutex_);
-    return map_[domain].emplace(op, std::forward<T>(data)).second;
+    auto result = map_[domain].try_emplace(op, std::forward<T>(data));
+    if (!result.second) result.first->second = std::forward<T>(data);
+    return result.second;
   }
 
   /* Remove { domain, op } from the journal. Return false if the entry did not exist.  */

projects/roctracer/src/core/roctracer.cpp

@@ -34,6 +34,7 @@
 #include <mutex>
 #include <stack>
 #include <unordered_map>
+#include <vector>
 
 #include "core/journal.h"
 #include "core/loader.h"
@@ -498,7 +499,7 @@ util::Logger::mutex_t util::Logger::mutex_;
 std::atomic<util::Logger*> util::Logger::instance_{};
 
 // Memory pool routines and primitives
-MemoryPool* memory_pool = nullptr;
+MemoryPool* default_memory_pool = nullptr;
 std::recursive_mutex memory_pool_mutex;
 
 // Stop status routines and primitives
@@ -801,8 +802,8 @@ PUBLIC_API roctracer_status_t roctracer_disable_callback() {
 // Return default pool and set new one if parameter pool is not NULL.
 PUBLIC_API roctracer_pool_t* roctracer_default_pool_expl(roctracer_pool_t* pool) {
   std::lock_guard lock(memory_pool_mutex);
-  roctracer_pool_t* p = reinterpret_cast<roctracer_pool_t*>(memory_pool);
-  if (pool != nullptr) memory_pool = reinterpret_cast<MemoryPool*>(pool);
+  roctracer_pool_t* p = reinterpret_cast<roctracer_pool_t*>(default_memory_pool);
+  if (pool != nullptr) default_memory_pool = reinterpret_cast<MemoryPool*>(pool);
   return p;
 }
 
@@ -811,7 +812,7 @@ PUBLIC_API roctracer_status_t roctracer_open_pool_expl(const roctracer_propertie
                                                        roctracer_pool_t** pool) {
   API_METHOD_PREFIX
   std::lock_guard lock(memory_pool_mutex);
-  if ((pool == nullptr) && (memory_pool != nullptr)) {
+  if ((pool == nullptr) && (default_memory_pool != nullptr)) {
     EXC_RAISING(ROCTRACER_STATUS_ERROR, "default pool already set");
   }
   MemoryPool* p = new MemoryPool(*properties);
@@ -819,25 +820,14 @@ PUBLIC_API roctracer_status_t roctracer_open_pool_expl(const roctracer_propertie
   if (pool != nullptr)
     *pool = p;
   else
-    memory_pool = p;
-  API_METHOD_SUFFIX
-}
-
-// Close memory pool
-PUBLIC_API roctracer_status_t roctracer_close_pool_expl(roctracer_pool_t* pool) {
-  API_METHOD_PREFIX
-  std::lock_guard lock(memory_pool_mutex);
-  roctracer_pool_t* ptr = (pool == nullptr) ? roctracer_default_pool() : pool;
-  MemoryPool* memory_pool = reinterpret_cast<MemoryPool*>(ptr);
-  delete (memory_pool);
-  if (pool == nullptr) memory_pool = nullptr;
+    default_memory_pool = p;
   API_METHOD_SUFFIX
 }
 
 // Enable activity records logging
 static roctracer_status_t roctracer_enable_activity_fun(roctracer_domain_t domain, uint32_t op,
                                                         roctracer_pool_t* pool) {
-  if (pool == nullptr) pool = roctracer_default_pool();
+  assert(pool != nullptr);
   switch (domain) {
     case ACTIVITY_DOMAIN_HSA_OPS: {
       if (op == HSA_OP_ID_COPY) {
@@ -897,6 +887,8 @@ static roctracer_status_t roctracer_enable_activity_fun(roctracer_domain_t domai
 
 static void roctracer_enable_activity_impl(roctracer_domain_t domain, uint32_t op,
                                            roctracer_pool_t* pool) {
+  if (pool == nullptr) pool = default_memory_pool;
+  if (pool == nullptr) EXC_RAISING(ROCTRACER_STATUS_ERROR, "no default pool");
   act_journal.Insert(domain, op, {pool});
   roctracer_enable_activity_fun(domain, op, pool);
 }
@@ -1011,12 +1003,35 @@ PUBLIC_API roctracer_status_t roctracer_disable_activity() {
   API_METHOD_SUFFIX
 }
 
+// Close memory pool
+PUBLIC_API roctracer_status_t roctracer_close_pool_expl(roctracer_pool_t* pool) {
+  API_METHOD_PREFIX
+  std::lock_guard lock(memory_pool_mutex);
+  if (pool == nullptr) pool = reinterpret_cast<roctracer_pool_t*>(default_memory_pool);
+  if (pool != nullptr) {
+    MemoryPool* p = reinterpret_cast<MemoryPool*>(pool);
+    if (p == default_memory_pool) default_memory_pool = nullptr;
+
+    // Disable any activities that specify the pool being deleted.
+    std::vector<std::pair<roctracer_domain_t, uint32_t>> ops;
+    act_journal.ForEach(
+        [&ops, pool](roctracer_domain_t domain, uint32_t op, const ActivityJournalData& data) {
+          if (pool == data.pool) ops.emplace_back(domain, op);
+          return true;
+        });
+    for (auto&& [domain, op] : ops) roctracer_disable_activity_impl(domain, op);
+
+    delete (p);
+  }
+  API_METHOD_SUFFIX
+}
+
 // Flush available activity records
 PUBLIC_API roctracer_status_t roctracer_flush_activity_expl(roctracer_pool_t* pool) {
   API_METHOD_PREFIX
   if (pool == nullptr) pool = roctracer_default_pool();
-  MemoryPool* memory_pool = reinterpret_cast<MemoryPool*>(pool);
-  if (memory_pool != nullptr) memory_pool->Flush();
+  MemoryPool* default_memory_pool = reinterpret_cast<MemoryPool*>(pool);
+  if (default_memory_pool != nullptr) default_memory_pool->Flush();
   API_METHOD_SUFFIX
 }