This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] aio: fix newp->running data race


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);


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]