From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=59222 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P6lhs-0001T2-Hc for qemu-devel@nongnu.org; Fri, 15 Oct 2010 10:56:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P6lhr-0004JE-6J for qemu-devel@nongnu.org; Fri, 15 Oct 2010 10:56:28 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:43897) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P6lhr-0004J1-3B for qemu-devel@nongnu.org; Fri, 15 Oct 2010 10:56:27 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by e8.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id o9FEfMYW012949 for ; Fri, 15 Oct 2010 10:41:22 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o9FEuPDU470428 for ; Fri, 15 Oct 2010 10:56:25 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o9FEuORX002721 for ; Fri, 15 Oct 2010 08:56:24 -0600 Message-ID: <4CB86B98.9070007@linux.vnet.ibm.com> Date: Fri, 15 Oct 2010 07:56:24 -0700 From: "Venkateswararao Jujjuri (JV)" MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/3] Introduce threadlets References: <20101013152921.21735.87339.stgit@localhost6.localdomain6> <20101013153110.21735.16669.stgit@localhost6.localdomain6> <4CB7736C.3040901@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Arun R Bharadwaj , qemu-devel@nongnu.org On 10/15/2010 2:52 AM, Stefan Hajnoczi wrote: > On Thu, Oct 14, 2010 at 10:17 PM, Venkateswararao Jujjuri (JV) > wrote: >> On 10/14/2010 2:02 AM, Stefan Hajnoczi wrote: >>> On Wed, Oct 13, 2010 at 4:31 PM, Arun R Bharadwaj >>>> +static void *threadlet_worker(void *data) >>>> +{ >>>> + ThreadletQueue *queue = data; >>>> + >>>> + qemu_mutex_lock(&(queue->lock)); >>>> + while (1) { >>>> + ThreadletWork *work; >>>> + int ret = 0; >>>> + >>>> + while (QTAILQ_EMPTY(&(queue->request_list)) && >>>> + (ret != ETIMEDOUT)) { >>>> + ret = qemu_cond_timedwait(&(queue->cond), >>>> + &(queue->lock), 10*100000); >>>> + } >>>> + >>>> + assert(queue->idle_threads != 0); >>>> + if (QTAILQ_EMPTY(&(queue->request_list))) { >>>> + if (queue->cur_threads > queue->min_threads) { >>>> + /* We retain the minimum number of threads */ >>>> + break; >>>> + } >>>> + } else { >>>> + work = QTAILQ_FIRST(&(queue->request_list)); >>>> + QTAILQ_REMOVE(&(queue->request_list), work, node); >>>> + >>>> + queue->idle_threads--; >>>> + qemu_mutex_unlock(&(queue->lock)); >>>> + >>>> + /* execute the work function */ >>>> + work->func(work); >>>> + >>>> + qemu_mutex_lock(&(queue->lock)); >>>> + queue->idle_threads++; >>>> + } >>>> + } >>>> + >>>> + queue->idle_threads--; >>>> + queue->cur_threads--; >>>> + qemu_mutex_unlock(&(queue->lock)); >>>> + >>>> + return NULL; >>>> +} >>>> + >>>> +static void spawn_threadlet(ThreadletQueue *queue) >>>> +{ >>>> + QemuThread thread; >>>> + >>>> + queue->cur_threads++; >>>> + queue->idle_threads++; >>>> + >>>> + qemu_thread_create(&thread, threadlet_worker, queue); >>>> +} >>>> + >>>> +/** >>>> + * submit_threadletwork_to_queue: Submit a new task to a private queue to be >>>> + * executed asynchronously. >>>> + * @queue: Per-subsystem private queue to which the new task needs >>>> + * to be submitted. >>>> + * @work: Contains information about the task that needs to be submitted. >>>> + */ >>>> +void submit_threadletwork_to_queue(ThreadletQueue *queue, ThreadletWork *work) >>>> +{ >>>> + qemu_mutex_lock(&(queue->lock)); >>>> + if (queue->idle_threads == 0 && queue->cur_threads < queue->max_threads) { >>>> + spawn_threadlet(queue); >>>> + } >>>> + QTAILQ_INSERT_TAIL(&(queue->request_list), work, node); >>>> + qemu_mutex_unlock(&(queue->lock)); >>>> + qemu_cond_signal(&(queue->cond)); >>> >>> Worker thread signalling and spawning has race conditions. See my >>> previous email: >>> >>> "There are race conditions here: >>> >>> 1. When a new threadlet is started because there are no idle threads, >>> qemu_cond_signal() may fire a blank because the threadlet isn't inside >>> qemu_cond_timedwait() yet. The result, the work item is deadlocked >>> until another thread grabs more work off the queue. If I'm reading >>> the code correctly this bug is currently present! >> >> Moving QTAILQ_INSERT_TAIL() ahead of spawn_threadlet() should take care of this >> issue >> right? > > I didn't read the code correctly. queue->lock is already held by > submit_threadletwork_to_queue() until after QTAILQ_INSERT_TAIL(). > threadlet_worker() will only enter its main loop once it acquires > queue->lock. Therefore the queue definitely has the work before the > spawned thread begins processing. > > The work is on the queue when threadlet_worker() enters its main loop, > so it will not need to wait on queue->cond but can process work > immediately. There is no spawn race condition. Correct...I too missed that. :) JV > > Stefan