From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59893) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUHCL-0006aD-OA for qemu-devel@nongnu.org; Tue, 15 May 2012 08:50:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SUH9F-0001q2-Do for qemu-devel@nongnu.org; Tue, 15 May 2012 08:47:08 -0400 Received: from mx.meyering.net ([88.168.87.75]:33788) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUH9F-0001pk-6G for qemu-devel@nongnu.org; Tue, 15 May 2012 08:46:41 -0400 From: Jim Meyering In-Reply-To: <4FB23F3E.4000700@redhat.com> (Kevin Wolf's message of "Tue, 15 May 2012 13:34:22 +0200") References: <87vcjx1y7k.fsf@rho.meyering.net> <4FB23F3E.4000700@redhat.com> Date: Tue, 15 May 2012 14:46:38 +0200 Message-ID: <87ehql1uj5.fsf@rho.meyering.net> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] posix-aio: don't set aiocb->ret/active outside critical section List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Paolo Bonzini , Anthony Liguori , qemu-devel@nongnu.org, Stefan Hajnoczi , Frediano Ziglio Kevin Wolf wrote: > Am 15.05.2012 13:27, schrieb Jim Meyering: >> >> Move code that sets aiocb->ret and aiocb->active into critical section. >> All other accesses are lock-guarded. Spotted by coverity. >> >> Signed-off-by: Jim Meyering >> --- >> I've included enough context to show one of the >> guarded uses in the following function. >> >> posix-aio-compat.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/posix-aio-compat.c b/posix-aio-compat.c >> index 68361f5..45511d4 100644 >> --- a/posix-aio-compat.c >> +++ b/posix-aio-compat.c >> @@ -421,37 +421,37 @@ static void spawn_thread(void) >> { >> cur_threads++; >> new_threads++; >> /* If there are threads being created, they will spawn new workers, so >> * we don't spend time creating many threads in a loop holding a mutex or >> * starving the current vcpu. >> * >> * If there are no idle threads, ask the main thread to create one, so we >> * inherit the correct affinity instead of the vcpu affinity. >> */ >> if (!pending_threads) { >> qemu_bh_schedule(new_thread_bh); >> } >> } >> >> static void qemu_paio_submit(struct qemu_paiocb *aiocb) >> { >> + mutex_lock(&lock); >> aiocb->ret = -EINPROGRESS; >> aiocb->active = 0; >> - mutex_lock(&lock); >> if (idle_threads == 0 && cur_threads < max_threads) >> spawn_thread(); >> QTAILQ_INSERT_TAIL(&request_list, aiocb, node); >> mutex_unlock(&lock); >> cond_signal(&cond); >> } > > This is just silencing coverity, not really fixing a bug, right? aiocb Yes. Given your explanation, it's obvious, now. > is inserted into request_lists only afterwards, and before it is in the > list, other threads can't find it. > > I'm just wondering why coverity doesn't complain about the callers of > qemu_paio_submit(), they access aiocb as well... It warned only about the setting of aiocb->ret (not about ->active), apparently because it failed to deduce that the lock is not actually required here, while several other ->ret accesses *are* guarded.