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: GNU C Library <libc-alpha at sourceware dot org>, drepper at gmail dot com, roland at gnu dot org
- Date: Wed, 18 May 2016 15:02:09 +0200
- Subject: Re: [PATCH] aio: fix newp->running data race
- Authentication-results: sourceware.org; auth=none
- References: <20160504140217 dot GZ2861 at var dot bordeaux dot inria dot fr>
Ping?
Samuel Thibault, on Wed 04 May 2016 16:02:17 +0200, wrote:
> Hello,
>
> As reported by helgrind:
>
> ==9363== Possible data race during write of size 4 at 0x6B45D18 by thread #5
> ==9363== Locks held: 2, at addresses 0x557A1E0 0x6A149E0
> ==9363== at 0x5375E0D: __aio_enqueue_request (in /lib/x86_64-linux-gnu/librt-2.21.so)
> ==9363== by 0x53764FD: aio_write (in /lib/x86_64-linux-gnu/librt-2.21.so)
>
> which is the write in
>
> if (result == 0)
> newp->running = running;
>
> ==9363== This conflicts with a previous read of size 4 by thread #6
> ==9363== Locks held: none
> ==9363== at 0x53758FE: handle_fildes_io (in /lib/x86_64-linux-gnu/librt-2.21.so)
>
> which is the read in
>
> /* Hopefully this request is marked as running. */
> assert (runp->running == allocated);
>
> So what is happening is that __aio_enqueue_request writes newp->running
> after having possibly started a thread worker to handle the request,
> thus in concurrence with the thread worker, which not only reads
> runp->running as shown here, but also writes to it later (and does not
> take __aio_requests_mutex since it's already given a request).
>
> But actually when we do start a new thread worker, there is no need
> to set newp->running, since it was already set by the "running =
> newp->running = allocated;" line along the thread creation. So to avoid
> the race we can just set newp->running in the cases where we do not
> start the thread.
>
> 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.
>
> diff --git a/sysdeps/pthread/aio_misc.c b/sysdeps/pthread/aio_misc.c
> index f55570d..faf139d 100644
> --- a/sysdeps/pthread/aio_misc.c
> +++ b/sysdeps/pthread/aio_misc.c
> @@ -453,7 +453,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 +470,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);
--
Samuel
<c> ya(ka|ma|to)* ca existe une fois sur 2 au japon, c'est facile ;-)
-+- #ens-mim au japon -+-