From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KffqR-00005N-By for qemu-devel@nongnu.org; Tue, 16 Sep 2008 15:04:15 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KffqP-0008Vs-NU for qemu-devel@nongnu.org; Tue, 16 Sep 2008 15:04:14 -0400 Received: from [199.232.76.173] (port=48449 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KffqP-0008Vp-Da for qemu-devel@nongnu.org; Tue, 16 Sep 2008 15:04:13 -0400 Received: from e2.ny.us.ibm.com ([32.97.182.142]:40718) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KffqO-0005ff-Tj for qemu-devel@nongnu.org; Tue, 16 Sep 2008 15:04:13 -0400 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by e2.ny.us.ibm.com (8.13.8/8.13.8) with ESMTP id m8GJ49TZ009248 for ; Tue, 16 Sep 2008 15:04:09 -0400 Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id m8GJ47p21105960 for ; Tue, 16 Sep 2008 15:04:08 -0400 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m8GJ41eo007937 for ; Tue, 16 Sep 2008 13:04:04 -0600 Message-ID: <48D002E9.3070400@us.ibm.com> Date: Tue, 16 Sep 2008 14:03:05 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Refactor AIO to allow multiple AIO implementations References: <1221582247-8886-1-git-send-email-aliguori@us.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Ryan Harper , qemu-devel@nongnu.org, kvm@vger.kernel.org Blue Swirl wrote: > On 9/16/08, Anthony Liguori wrote: > >> This patch refactors the AIO layer to allow multiple AIO implementations. It's >> only possible because of the recent signalfd() patch. >> > > >> +/* This is a simple lock used to protect the aio_handlers list. Specifically, >> + * it's used to ensure that no callbacks are removed while we're walking and >> + * dispatching callbacks. >> + */ >> +static int walking_handlers; >> > > Shouldn't this be volatile and/or atomic_t? > No, because this is just used to protect the aio_handlers list within the same thread. Here's the scenario we're protecting: Walk aio_handlers list => call handler => handler calls qemu_aio_set_fd_handler() to delete aio entry Since we're walking the list, we can't delete an entry while walking. This is the same problem we have in vl.c with the IOHandler list. Unlike the IOHandler list, we don't walk the aio_handler list often enough to just set a flag on the entry. In the case where qemu_aio_set_fd_handler() is called from something other than a handler callback, we need to be able to directly delete the entry element. This is why we use the "lock", to detect that case. It's not a real lock, because two threads are never trying to access it at the same time. This is why we don't need volatile, atomic_t, or the real locking primitives. Regards, Anthony Liguori > Just wondering, why don't you use real locking operations, for example > those in qemu-lock.h? >