Use a RAII approach for linux signal blocking

Author: sophimao

include/acl_thread.h

@@ -67,20 +67,13 @@ static inline void acl_sig_finished() {
 }
 
 // Blocking/Unblocking signals (Only implemented for Linux)
-#ifdef __linux__
-extern ACL_TLS sigset_t acl_sigset;
-static inline void acl_sig_block_signals() {
-  sigset_t mask;
-  if (sigfillset(&mask))
-    assert(0 && "Error in creating signal mask in status handler");
-  if (pthread_sigmask(SIG_BLOCK, &mask, &acl_sigset))
-    assert(0 && "Error in blocking signals in status handler");
-}
-static inline void acl_sig_unblock_signals() {
-  if (pthread_sigmask(SIG_SETMASK, &acl_sigset, NULL))
-    assert(0 && "Error in unblocking signals in status handler");
-}
-#endif
+class acl_signal_blocker {
+public:
+  acl_signal_blocker &operator=(const acl_signal_blocker &) = delete;
+  acl_signal_blocker(const acl_signal_blocker &) = delete;
+  acl_signal_blocker();
+  ~acl_signal_blocker();
+};
 
 // -- global lock functions --
 

src/acl_hal_mmd.cpp

@@ -1985,13 +1985,13 @@ int acl_hal_mmd_program_device(unsigned int physical_device_id,
 
 void acl_hal_mmd_kernel_interrupt(int handle_in, void *user_data) {
   unsigned physical_device_id;
-#ifdef __linux__
+
   // Callbacks received from non-dma transfers.
   // (those calls are not initiated by a signal handler, so we need to block all
   // signals here to avoid simultaneous calls to signal handler.)
-  acl_sig_block_signals(); // Call before acl_sig_started. Must call
-                           // acl_sig_unblock_signals after acl_sig_finished.
-#endif
+  // Must instantiate before acl_sig_started, destruct after acl_sig_finished.
+  acl_signal_blocker sig_blocker;
+
   acl_sig_started();
   // NOTE: all exit points of this function must first call acl_sig_finished()
 
@@ -2005,10 +2005,6 @@ void acl_hal_mmd_kernel_interrupt(int handle_in, void *user_data) {
     assert(acl_kernel_if_is_valid(&kern[physical_device_id]));
     acl_kernel_if_update_status(&kern[physical_device_id]);
     acl_sig_finished();
-#ifdef __linux__
-    // Unblocking the signals we blocked
-    acl_sig_unblock_signals();
-#endif
     return;
   }
 
@@ -2021,14 +2017,13 @@ void acl_hal_mmd_device_interrupt(int handle_in,
                                   aocl_mmd_interrupt_info *data_in,
                                   void *user_data) {
   unsigned physical_device_id;
-#ifdef __linux__
+
   // Callbacks received from non-dma transfers.
-  //(those calls are not initiated by a signal handler, so we need to block all
-  // signals
-  // here to avoid simultaneous calls to signal handler.)
-  acl_sig_block_signals(); // Call before acl_sig_started. Must call
-                           // acl_sig_unblock_signals after acl_sig_finished.
-#endif
+  // (those calls are not initiated by a signal handler, so we need to block all
+  // signals here to avoid simultaneous calls to signal handler.)
+  // Must instantiate before acl_sig_started, destruct after acl_sig_finished.
+  acl_signal_blocker sig_blocker;
+
   acl_sig_started();
   // NOTE: all exit points of this function must first call acl_sig_finished()
 
@@ -2042,10 +2037,6 @@ void acl_hal_mmd_device_interrupt(int handle_in,
     acl_device_update_fn(physical_device_id, data_in->exception_type,
                          data_in->user_private_info, data_in->user_cb);
     acl_sig_finished();
-#ifdef __linux__
-    // Unblocking the signals we blocked
-    acl_sig_unblock_signals();
-#endif
     return;
   }
 
@@ -2056,14 +2047,12 @@ void acl_hal_mmd_device_interrupt(int handle_in,
 
 void acl_hal_mmd_status_handler(int handle, void *user_data, aocl_mmd_op_t op,
                                 int status) {
-#ifdef __linux__
   // Callbacks received from non-dma transfers.
-  //(those calls are not initiated by a signal handler, so we need to block all
-  // signals)
-  // here to avoid simultaneous  calls to signal handler.)
-  acl_sig_block_signals(); // Call before acl_sig_started. Must call
-                           // acl_sig_unblock_signals after acl_sig_finished.
-#endif
+  // (those calls are not initiated by a signal handler, so we need to block all
+  // signals here to avoid simultaneous calls to signal handler.)
+  // Must instantiate before acl_sig_started, destruct after acl_sig_finished.
+  acl_signal_blocker sig_blocker;
+
   acl_sig_started();
   // NOTE: all exit points of this function must first call acl_sig_finished()
   // Removing Windows warning
@@ -2073,10 +2062,6 @@ void acl_hal_mmd_status_handler(int handle, void *user_data, aocl_mmd_op_t op,
   acl_event_update_fn((cl_event)op, CL_COMPLETE);
 
   acl_sig_finished();
-#ifdef __linux__
-  // Unblocking the signals we blocked
-  acl_sig_unblock_signals();
-#endif
 }
 
 void acl_hal_mmd_register_callbacks(

src/acl_kernel_if.cpp

@@ -400,42 +400,30 @@ static uintptr_t acl_kernel_cra_set_segment_rom(acl_kernel_if *kern,
 
 static int acl_kernel_cra_read(acl_kernel_if *kern, unsigned int accel_id,
                                unsigned int addr, unsigned int *val) {
-  int result = 0;
   assert(kern->cra_ring_root_exist);
-#ifdef __linux__
-  acl_sig_block_signals();
-#endif
-  {
-    std::lock_guard<std::mutex> lock(kern->segment_mutex);
-    uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
-    acl_assert_locked_or_sig();
-    result = acl_kernel_if_read_32b(
-        kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
-  }
-#ifdef __linux__
-  acl_sig_unblock_signals();
-#endif
-  return result;
+
+  // Need to block signals before acquiring mutex and unblock after releasing
+  acl_signal_blocker sig_blocker;
+  std::lock_guard<std::mutex> lock(kern->segment_mutex);
+
+  uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
+  acl_assert_locked_or_sig();
+  return acl_kernel_if_read_32b(
+      kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
 }
 
 int acl_kernel_cra_read_64b(acl_kernel_if *kern, unsigned int accel_id,
                             unsigned int addr, uint64_t *val) {
-  int result = 0;
   assert(kern->cra_ring_root_exist);
-#ifdef __linux__
-  acl_sig_block_signals();
-#endif
-  {
-    std::lock_guard<std::mutex> lock(kern->segment_mutex);
-    uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
-    acl_assert_locked_or_sig();
-    result = acl_kernel_if_read_64b(
-        kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
-  }
-#ifdef __linux__
-  acl_sig_unblock_signals();
-#endif
-  return result;
+
+  // Need to block signals before acquiring mutex and unblock after releasing
+  acl_signal_blocker sig_blocker;
+  std::lock_guard<std::mutex> lock(kern->segment_mutex);
+
+  uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
+  acl_assert_locked_or_sig();
+  return acl_kernel_if_read_64b(
+      kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
 }
 
 // Read 32b from kernel ROM
@@ -483,95 +471,76 @@ static int acl_kernel_rom_cra_read_block(acl_kernel_if *kern, unsigned int addr,
 
 static int acl_kernel_cra_write(acl_kernel_if *kern, unsigned int accel_id,
                                 unsigned int addr, unsigned int val) {
-  int result = 0;
   assert(kern->cra_ring_root_exist);
-#ifdef __linux__
-  acl_sig_block_signals();
-#endif
-  {
-    std::lock_guard<std::mutex> lock(kern->segment_mutex);
-    uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
-    acl_assert_locked_or_sig();
-    result = acl_kernel_if_write_32b(
-        kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
-  }
-#ifdef __linux__
-  acl_sig_unblock_signals();
-#endif
-  return result;
+
+  // Need to block signals before acquiring mutex and unblock after releasing
+  acl_signal_blocker sig_blocker;
+  std::lock_guard<std::mutex> lock(kern->segment_mutex);
+
+  uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
+  acl_assert_locked_or_sig();
+  return acl_kernel_if_write_32b(
+      kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
 }
 
 static int acl_kernel_cra_write_64b(acl_kernel_if *kern, unsigned int accel_id,
                                     unsigned int addr, uint64_t val) {
-  int result = 0;
   assert(kern->cra_ring_root_exist);
-#ifdef __linux__
-  acl_sig_block_signals();
-#endif
-  {
-    std::lock_guard<std::mutex> lock(kern->segment_mutex);
-    uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
-    acl_assert_locked();
-    result = acl_kernel_if_write_64b(
-        kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
-  }
-#ifdef __linux__
-  acl_sig_unblock_signals();
-#endif
-  return result;
+
+  // Need to block signals before acquiring mutex and unblock after releasing
+  acl_signal_blocker sig_blocker;
+  std::lock_guard<std::mutex> lock(kern->segment_mutex);
+
+  uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
+  acl_assert_locked();
+  return acl_kernel_if_write_64b(
+      kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val);
 }
 
 static int acl_kernel_cra_write_block(acl_kernel_if *kern,
                                       unsigned int accel_id, unsigned int addr,
                                       unsigned int *val, size_t size) {
-  int result = 0;
   assert(kern->cra_ring_root_exist);
-#ifdef __linux__
-  acl_sig_block_signals();
-#endif
-  {
-    std::lock_guard<std::mutex> lock(kern->segment_mutex);
-    uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
-    uintptr_t logical_addr =
-        kern->accel_csr[accel_id].address + addr - OFFSET_KERNEL_CRA;
-    uintptr_t segment = logical_addr & ((size_t)0 - (KERNEL_CRA_SEGMENT_SIZE));
-
-    uintptr_t logical_addr_end =
-        kern->accel_csr[accel_id].address + addr + size - OFFSET_KERNEL_CRA;
-    uintptr_t segment_end =
-        logical_addr_end & ((size_t)0 - (KERNEL_CRA_SEGMENT_SIZE));
-
-    unsigned int step = 0;
-    if (segment != segment_end) {
-      ACL_KERNEL_IF_DEBUG_MSG_VERBOSE(
-          kern, 2, ":: Segment change during block write detected.\n");
-      while (step < size) {
-        segment =
-            (logical_addr + step) & ((size_t)0 - (KERNEL_CRA_SEGMENT_SIZE));
-        if (kern->cur_segment != segment) {
-          acl_kernel_if_write_block(
-              kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val,
-              step);
-          segment_offset =
-              acl_kernel_cra_set_segment(kern, accel_id, addr + step);
-          logical_addr = kern->accel_csr[accel_id].address + addr + step -
-                         OFFSET_KERNEL_CRA;
-          val += step;
-          size -= step;
-          step = 0;
-        } else {
-          step += (unsigned)sizeof(int);
-        }
+
+  // Need to block signals before acquiring mutex and unblock after releasing
+  acl_signal_blocker sig_blocker;
+  std::lock_guard<std::mutex> lock(kern->segment_mutex);
+
+  uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
+  uintptr_t logical_addr =
+      kern->accel_csr[accel_id].address + addr - OFFSET_KERNEL_CRA;
+  uintptr_t segment = logical_addr & ((size_t)0 - (KERNEL_CRA_SEGMENT_SIZE));
+
+  uintptr_t logical_addr_end =
+      kern->accel_csr[accel_id].address + addr + size - OFFSET_KERNEL_CRA;
+  uintptr_t segment_end =
+      logical_addr_end & ((size_t)0 - (KERNEL_CRA_SEGMENT_SIZE));
+
+  unsigned int step = 0;
+  if (segment != segment_end) {
+    ACL_KERNEL_IF_DEBUG_MSG_VERBOSE(
+        kern, 2, ":: Segment change during block write detected.\n");
+    while (step < size) {
+      segment = (logical_addr + step) & ((size_t)0 - (KERNEL_CRA_SEGMENT_SIZE));
+      if (kern->cur_segment != segment) {
+        acl_kernel_if_write_block(
+            kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val,
+            step);
+        segment_offset =
+            acl_kernel_cra_set_segment(kern, accel_id, addr + step);
+        logical_addr =
+            kern->accel_csr[accel_id].address + addr + step - OFFSET_KERNEL_CRA;
+        val += step;
+        size -= step;
+        step = 0;
+      } else {
+        step += (unsigned)sizeof(int);
       }
     }
-    result = acl_kernel_if_write_block(
-        kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val,
-        size);
   }
-#ifdef __linux__
-  acl_sig_unblock_signals();
-#endif
-  return result;
+
+  return acl_kernel_if_write_block(
+      kern, (unsigned)OFFSET_KERNEL_CRA + (unsigned)segment_offset, val, size);
 }
 
 // Private utility function to issue a command to the profile hardware

src/acl_thread.cpp

@@ -135,6 +135,27 @@ static void l_init_once() {
 
 #endif // !LINUX
 
+// Blocking/Unblocking signals (Actual implementation only for Linux)
+acl_signal_blocker::acl_signal_blocker() {
+#ifdef __linux__
+  sigset_t mask;
+  if (sigfillset(&mask)) {
+    assert(0 && "Error in creating signal mask in status handler");
+  }
+  if (pthread_sigmask(SIG_BLOCK, &mask, &acl_sigset)) {
+    assert(0 && "Error in blocking signals in status handler");
+  }
+#endif
+}
+
+acl_signal_blocker::~acl_signal_blocker() {
+#ifdef __linux__
+  if (pthread_sigmask(SIG_SETMASK, &acl_sigset, NULL)) {
+    assert(0 && "Error in unblocking signals in status handler");
+  }
+#endif
+}
+
 // Current thread releases mutex lock and sleeps briefly to allow other threads
 // a chance to execute. This function is useful for multithreaded hosts with
 // e.g. polling BSPs (using yield) to prevent one thread from hogging the mutex