From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50155 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OOuQG-0008Sa-Bo for qemu-devel@nongnu.org; Wed, 16 Jun 2010 11:21:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OOuQE-0006Ay-Uu for qemu-devel@nongnu.org; Wed, 16 Jun 2010 11:21:00 -0400 Received: from e5.ny.us.ibm.com ([32.97.182.145]:33931) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OOuQE-0006AP-S9 for qemu-devel@nongnu.org; Wed, 16 Jun 2010 11:20:58 -0400 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by e5.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id o5GF3uCa030756 for ; Wed, 16 Jun 2010 11:03:57 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o5GFKpRw1036514 for ; Wed, 16 Jun 2010 11:20:51 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o5GFKoDY011814 for ; Wed, 16 Jun 2010 12:20:51 -0300 Message-ID: <4C18EBC4.4040603@linux.vnet.ibm.com> Date: Wed, 16 Jun 2010 10:20:36 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH V4 2/3] qemu: Generic task offloading framework: threadlets References: <20100616115404.10988.62371.stgit@localhost.localdomain> <20100616115656.10988.96529.stgit@localhost.localdomain> <4C18C4C8.8090901@redhat.com> <20100616142236.GA20052@shareable.org> <4C18DFD7.1090102@redhat.com> <4C18E1E8.3030606@linux.vnet.ibm.com> <4C18E52B.9010600@redhat.com> In-Reply-To: <4C18E52B.9010600@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Gautham R Shenoy , Qemu-development List , "Aneesh Kumar K.V" , Corentin Chary , Avi Kivity On 06/16/2010 09:52 AM, Paolo Bonzini wrote: > On 06/16/2010 04:38 PM, Anthony Liguori wrote: >> On 06/16/2010 09:29 AM, Paolo Bonzini wrote: >>> On 06/16/2010 04:22 PM, Jamie Lokier wrote: >>>> Paolo Bonzini wrote: >>>>> These should be (at least for now) block-obj-$(CONFIG_POSIX). >>>>> >>>>>> + while (QTAILQ_EMPTY(&(queue->request_list))&& >>>>>> + (ret != ETIMEDOUT)) { >>>>>> + ret = qemu_cond_timedwait(&(queue->cond), >>>>>> + &(queue->lock), 10*100000); >>>>>> + } >>>>> >>>>> Using qemu_cond_timedwait is a hack for not properly broadcasting the >>>>> condvar in flush_threadlet_queue. >>>> >>>> Are you sure? It looks like it also expires idle threads after a >>>> fixed amount of idle time. >>> >>> Unnecessary idle threads are immediately expired as soon as the >>> threadlet exits if ncecessary, since here >> >> If a threadlet is waiting to consume more work, unless we do a >> pthread_cancel (I dislike cancellation) it will keep waiting until it >> gets more work (which would mean it's not actually idle)... > > Agreed---no cancellation, please. > > BTW it's obviously okay with signaling the condition when a threadlet > is submitted. But when something affects all queue's workers > (flush_threadlet_queue) you want a broadcast and using expiration as a > substitute is fishy. IMHO, there shouldn't be a need for flush_threadlet_queue. It doesn't look used in the aio conversion and if virtio-9p needs it, I suspect something is wrong. Regards, Anthony Liguori >>> + queue->idle_threads++; >>> + >>> +check_exit: >>> + if (queue->exit || ((queue->idle_threads > 0) && >>> + (queue->cur_threads > queue->min_threads))) { >>> + /* We exit the queue or we retain minimum number of threads */ >>> + break; >>> + } >>> >>> queue->idle_threads > 0 will always be true (so maybe that should be >>> changed into an assertion: "this thread is idle, so there must be idle >>> threads"). >> >> queue->exit could be true though so it's necessary to at least check >> that condition. > > Yes, of course. The correct test should be: > > if (queue->exit || queue->cur_threads > queue->min_threads) > > But queue->idle_threads will be > 0 even if coming via the goto (which > should be eliminated). > > Or maybe no. After flushing you still want min_threads threads to > run. The correct thing then would be: > > do { > ... > assert (queue->idle_threads > 0); > if (queue->exit) { > /* Threads waiting on the barrier cannot do work. */ > queue->idle_threads--; > qemu_mutex_unlock(&(queue->lock)); > qemu_barrier_wait(&queue->barr); > qemu_mutex_lock(&(queue->lock)); > queue->idle_threads++; > } > } while (queue->cur_threads <= queue->min_threads); > > queue->idle_threads--; > queue->cur_threads--; > qemu_mutex_unlock(&queue->lock); > return NULL; > > So, if min_threads were changed, broadcasting the condition would be > enough to exit unwanted threads one at a time, as soon as it grabs the > lock. > > Paolo