diff mbox series

[09/13] AMD GCN libgomp plugin queue-full condition locking fix

Message ID ef9ad097d1310d33e1ef6d0a7cbd5c15ea59d7f3.1573849744.git.julian@codesourcery.com
State New
Headers show
Series AMD GCN worker partitioning support | expand

Commit Message

Julian Brown Nov. 15, 2019, 9:44 p.m. UTC 
This patch corrects a possible race condition in locking for the
asynchronous queue-full condition check in the AMD GCN libgomp plugin.

OK?

Julian

ChangeLog

	libgomp/
	* plugin/plugin-gcn.c (wait_for_queue_nonfull): Don't lock/unlock
	aq->mutex here.
	(queue_push_launch): Lock aq->mutex before calling
	wait_for_queue_nonfull.
	(queue_push_callback): Likewise.
	(queue_push_asyncwait): Likewise.
	(queue_push_placeholder): Likewise.
---
 libgomp/plugin/plugin-gcn.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

Comments

Andrew Stubbs Nov. 18, 2019, 11:05 a.m. UTC | #1 
On 15/11/2019 21:44, Julian Brown wrote:
> @@ -2732,13 +2732,9 @@ wait_for_queue_nonfull (struct goacc_asyncqueue *aq)
>   {
>     if (aq->queue_n == ASYNC_QUEUE_SIZE)
>       {
> -      pthread_mutex_lock (&aq->mutex);
> -
>         /* Queue is full.  Wait for it to not be full.  */
>         while (aq->queue_n == ASYNC_QUEUE_SIZE)
>   	pthread_cond_wait (&aq->queue_cond_out, &aq->mutex);
> -
> -      pthread_mutex_unlock (&aq->mutex);
>       }
>   }

If wait_for_queue_nonfull requires the mutex locked on entry then the 
comment above the function should say so.

Otherwise this looks fine.

Andrew
diff mbox series

Patch

diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
index eb016e3fcd6..c4347dfa45d 100644
--- a/libgomp/plugin/plugin-gcn.c
+++ b/libgomp/plugin/plugin-gcn.c
@@ -2732,13 +2732,9 @@  wait_for_queue_nonfull (struct goacc_asyncqueue *aq)
 {
   if (aq->queue_n == ASYNC_QUEUE_SIZE)
     {
-      pthread_mutex_lock (&aq->mutex);
-
       /* Queue is full.  Wait for it to not be full.  */
       while (aq->queue_n == ASYNC_QUEUE_SIZE)
 	pthread_cond_wait (&aq->queue_cond_out, &aq->mutex);
-
-      pthread_mutex_unlock (&aq->mutex);
     }
 }
 
@@ -2752,10 +2748,10 @@  queue_push_launch (struct goacc_asyncqueue *aq, struct kernel_info *kernel,
 {
   assert (aq->agent == kernel->agent);
 
-  wait_for_queue_nonfull (aq);
-
   pthread_mutex_lock (&aq->mutex);
 
+  wait_for_queue_nonfull (aq);
+
   int queue_last = ((aq->queue_first + aq->queue_n)
 		    % ASYNC_QUEUE_SIZE);
   if (DEBUG_QUEUES)
@@ -2785,10 +2781,10 @@  static void
 queue_push_callback (struct goacc_asyncqueue *aq, void (*fn)(void *),
 		     void *data)
 {
-  wait_for_queue_nonfull (aq);
-
   pthread_mutex_lock (&aq->mutex);
 
+  wait_for_queue_nonfull (aq);
+
   int queue_last = ((aq->queue_first + aq->queue_n)
 		    % ASYNC_QUEUE_SIZE);
   if (DEBUG_QUEUES)
@@ -2818,10 +2814,10 @@  static void
 queue_push_asyncwait (struct goacc_asyncqueue *aq,
 		      struct placeholder *placeholderp)
 {
-  wait_for_queue_nonfull (aq);
-
   pthread_mutex_lock (&aq->mutex);
 
+  wait_for_queue_nonfull (aq);
+
   int queue_last = ((aq->queue_first + aq->queue_n) % ASYNC_QUEUE_SIZE);
   if (DEBUG_QUEUES)
     GCN_DEBUG ("queue_push_asyncwait %d:%d: at %i\n", aq->agent->device_id,
@@ -2849,10 +2845,10 @@  queue_push_placeholder (struct goacc_asyncqueue *aq)
 {
   struct placeholder *placeholderp;
 
-  wait_for_queue_nonfull (aq);
-
   pthread_mutex_lock (&aq->mutex);
 
+  wait_for_queue_nonfull (aq);
+
   int queue_last = ((aq->queue_first + aq->queue_n) % ASYNC_QUEUE_SIZE);
   if (DEBUG_QUEUES)
     GCN_DEBUG ("queue_push_placeholder %d:%d: at %i\n", aq->agent->device_id,