From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Lkpds-0001FC-8Q for qemu-devel@nongnu.org; Fri, 20 Mar 2009 21:04:52 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Lkpdn-00018X-4A for qemu-devel@nongnu.org; Fri, 20 Mar 2009 21:04:51 -0400 Received: from [199.232.76.173] (port=59594 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lkpdm-00018K-R2 for qemu-devel@nongnu.org; Fri, 20 Mar 2009 21:04:46 -0400 Received: from yw-out-1718.google.com ([74.125.46.156]:53657) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Lkpdm-0001Cm-AM for qemu-devel@nongnu.org; Fri, 20 Mar 2009 21:04:46 -0400 Received: by yw-out-1718.google.com with SMTP id 9so776628ywk.82 for ; Fri, 20 Mar 2009 18:04:45 -0700 (PDT) Message-ID: <49C43D29.7000206@codemonkey.ws> Date: Fri, 20 Mar 2009 20:04:41 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <20090319145705.988576615@localhost.localdomain> <20090319150537.910101569@localhost.localdomain> <49C3D5D5.4020006@codemonkey.ws> <20090321000617.GA26654@amt.cnet> In-Reply-To: <20090321000617.GA26654@amt.cnet> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [patch 2/7] qemu: separate thread for io Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcelo Tosatti Cc: qemu-devel@nongnu.org Marcelo Tosatti wrote: > On Fri, Mar 20, 2009 at 12:43:49PM -0500, Anthony Liguori wrote: > >>> + qemu_mutex_lock(&qemu_fair_mutex); >>> + qemu_mutex_unlock(&qemu_fair_mutex); >>> >>> >> This is extremely subtle. I think it can be made a bit more explicit >> via something like: >> >> int generation = qemu_io_generation() + 1; >> >> qemu_mutex_unlock(&qemu_global_mutex); >> >> if (timeout && !has_work(env)) >> wait_signal(timeout); >> >> qemu_mutex_lock(&qemu_global_mutex) >> >> while (qemu_io_generation() < generation) >> qemu_cond_wait(&qemu_io_generation_cond, &qemu_global_mutex); >> >> Then, in main_loop_wait(): >> >> >>> void main_loop_wait(int timeout) >>> { >>> IOHandlerRecord *ioh; >>> @@ -3660,7 +3806,7 @@ void main_loop_wait(int timeout) >>> */ >>> qemu_mutex_unlock(&qemu_global_mutex); >>> ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv); >>> - qemu_mutex_lock(&qemu_global_mutex); >>> >>> >> qemu_mutex_lock(&qemu_global_mutex); >> qemu_io_generation_add(); >> qemu_cond_signal(&qemu_io_generation_cond); >> >> This will ensure that the TCG loop backs off of qemu_global_mutex for at >> least one main_loop_wait() iteration. >> > > I don't see much benefit. It has the disadvantage of introducing more > state (the generation counter, the conditional), and the potential > problems associated with this state such as: > > - If there is no event for the iothread to process, TCG will throttle > unnecessarily (i can't see how that could happen, but is a > possibility) until some event breaks it out of select() thus > increasing the generation counter. > Right, you have to couple this with a signal sent from the TCG thread to the io thread. And yeah, signals are not 100% reliable. > - Its possible to miss signals (again, I can't see in happening > in the scheme you suggest), but.. > > Also note there is no need to be completly fair. It is fine to > eventually be unfair. > > Do you worry about interaction between the two locks? Can make the lock > ordering documented. > I need to think about this a bit more. I understand the idea behind qemu_signal_lock() a little better now. Since we know that the IO thread is trying to run in TCG (because we sent a signal to it), I wonder if we can use that as an indicator that we have to let the IO thread run for a bit. BTW, your patches lack commit messages and Signed-off-bys. Are you not ready for having them committed? I know that we still have to figure out Windows support but do you know of any other show stoppers? Have you thought about how this is going to affect kvm-userspace? Do you think we can eliminate the io threading code in kvm-userspace after this goes in? Regards, Anthony Liguori > Will spend some time thinking about this, need to take multiple > "starved" threads into account. > > Thanks. > >