diff mbox

aio: fix newp->running data race

Message ID 20170803164217.y2hzvwiudgnansbt@var.youpi.perso.aquilenet.fr
State New
Headers show

Commit Message

Samuel Thibault Aug. 3, 2017, 4:42 p.m. UTC 
Hello,

Ping?

Samuel

Samuel Thibault, on mer. 17 août 2016 14:49:29 +0200, wrote:
> Torvald Riegel, on Fri 08 Jul 2016 18:05:15 +0200, wrote:
> > On Fri, 2016-07-08 at 16:28 +0200, Samuel Thibault wrote:
> > > Ping?
> > 
> > I've just looked quickly at the patch.  Fixes for concurrency issues
> > should be accompanied with detailed comments, so that other developers
> > can easily grasp how synchronization is supposed to work.
> 
> Ok.  How about this?

	* sysdeps/pthread/aio_misc.c (__aio_enqueue_request): Do not write
	`running` field of `newp` when a thread was started to process it,
	since that thread will not take `__aio_requests_mutex`, and the field
	already has the proper value actually.
	* sysdeps/pthread/aio_misc.h: Add a notes about concurrency of
	aio requests.
diff mbox

Patch

diff --git a/sysdeps/pthread/aio_misc.c b/sysdeps/pthread/aio_misc.c
index f55570d..00b2feb 100644
--- a/sysdeps/pthread/aio_misc.c
+++ b/sysdeps/pthread/aio_misc.c
@@ -435,7 +435,9 @@  __aio_enqueue_request (aiocb_union *aiocbp, int operation)
 	  if (result == 0)
 	    /* We managed to enqueue the request.  All errors which can
 	       happen now can be recognized by calls to `aio_return' and
-	       `aio_error'.  */
+	       `aio_error'.  From here, newp must not be modified any more
+	       since a helper thread could be already working on it.
+	       */
 	    ++nthreads;
 	  else
 	    {
@@ -453,7 +455,11 @@  __aio_enqueue_request (aiocb_union *aiocbp, int operation)
 		result = 0;
 	    }
 	}
+      else
+	newp->running = running;
     }
+  else
+    newp->running = running;
 
   /* Enqueue the request in the run queue if it is not yet running.  */
   if (running == yes && result == 0)
@@ -466,9 +472,7 @@  __aio_enqueue_request (aiocb_union *aiocbp, int operation)
 	pthread_cond_signal (&__aio_new_request_notification);
     }
 
-  if (result == 0)
-    newp->running = running;
-  else
+  if (result != 0)
     {
       /* Something went wrong.  */
       __aio_free_request (newp);
diff --git a/sysdeps/pthread/aio_misc.h b/sysdeps/pthread/aio_misc.h
index e042998..66ef3b1 100644
--- a/sysdeps/pthread/aio_misc.h
+++ b/sysdeps/pthread/aio_misc.h
@@ -72,6 +72,18 @@  enum
   done
 };
 
+/* CONCURRENCY NOTES:
+
+   __aio_requests_mutex only protects the request list.  The requests
+   themselves are not protected, since the producer is alone producing it
+   before pushing it to the list, and the consumer is alone working on it after
+   popping from the list.
+
+   Notably, when __aio_create_helper_thread is used, it directly gives the
+   request to the thread, and thus no lock is taken.  Consequently, the
+   producer must not access the request after calling
+   __aio_create_helper_thread since the consumer will immediately work on it
+   without taking __aio_requests_mutex.  */
 
 /* Used to queue requests..  */
 struct requestlist