This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] aio: fix newp->running data race
- From: Samuel Thibault <samuel dot thibault at ens-lyon dot org>
- To: Torvald Riegel <triegel at redhat dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Wed, 17 Aug 2016 14:49:29 +0200
- Subject: Re: [PATCH] aio: fix newp->running data race
- Authentication-results: sourceware.org; auth=none
- References: <20160504140217.GZ2861@var.bordeaux.inria.fr> <20160708142810.GF13644@var.bordeaux.inria.fr> <1467993915.3700.187.camel@localhost.localdomain>
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.
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