aio: fix newp->running data race
Commit Message
Hello,
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?
Samuel
* 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.
@@ -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);
@@ -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