From patchwork Thu Aug 3 16:42:17 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Samuel Thibault X-Patchwork-Id: 21910 Received: (qmail 112076 invoked by alias); 3 Aug 2017 16:42:23 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 110880 invoked by uid 89); 3 Aug 2017 16:42:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_NEUTRAL autolearn=ham version=3.3.2 spammy=NOTES, HContent-Transfer-Encoding:8bit X-HELO: mail3-relais-sop.national.inria.fr Date: Thu, 3 Aug 2017 18:42:17 +0200 From: Samuel Thibault To: Torvald Riegel Cc: GNU C Library Subject: Re: [PATCH] aio: fix newp->running data race Message-ID: <20170803164217.y2hzvwiudgnansbt@var.youpi.perso.aquilenet.fr> References: <20160504140217.GZ2861@var.bordeaux.inria.fr> <20160708142810.GF13644@var.bordeaux.inria.fr> <1467993915.3700.187.camel@localhost.localdomain> <20160817124929.GW5802@var> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160817124929.GW5802@var> User-Agent: NeoMutt/20170113 (1.7.2) 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 --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