From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKy18-0006Aw-Fw for qemu-devel@nongnu.org; Fri, 13 May 2011 15:27:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QKy17-0003h5-9P for qemu-devel@nongnu.org; Fri, 13 May 2011 15:27:18 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:56114) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKy16-0003gJ-Jv for qemu-devel@nongnu.org; Fri, 13 May 2011 15:27:17 -0400 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [202.81.31.245]) by e23smtp06.au.ibm.com (8.14.4/8.13.1) with ESMTP id p4DJQeKP000698 for ; Sat, 14 May 2011 05:26:40 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p4DJR3X8483364 for ; Sat, 14 May 2011 05:27:03 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p4DJR3Vl010102 for ; Sat, 14 May 2011 05:27:03 +1000 From: "Aneesh Kumar K.V" In-Reply-To: <20110513085503.GA23158@stefanha-thinkpad.localdomain> References: <1305233867-4367-1-git-send-email-jvrao@linux.vnet.ibm.com> <20110513085503.GA23158@stefanha-thinkpad.localdomain> Date: Sat, 14 May 2011 00:56:57 +0530 Message-ID: <87sjsio0zi.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [0/25] Async threading for VirtFS using glib threads & coroutines. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , "Venkateswararao Jujjuri (JV)" Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com On Fri, 13 May 2011 09:55:03 +0100, Stefan Hajnoczi wrote: > On Thu, May 12, 2011 at 01:57:22PM -0700, Venkateswararao Jujjuri (JV) wrote: > > VirtFS (fileserver base on 9P) performs many blocking system calls in the > > vCPU context. This effort is to move the blocking calls out of vCPU/IO > > thread context, into asynchronous threads. > > > > Anthony's " Add hard build dependency on glib" patch and > > Kevin/Stefan's coroutine effort is a prerequisite. > > > > This patch set contains: > > - Converting all 9pfs calls into coroutines. > > - Each 9P operation will be modified for: > > - Remove post* functions. These are our call back functions which makes > > the code very hard to read. Now with coroutines, we can achieve the same > > state machine model with nice sequential code flow. > > - Move errno access near to the local_syscall() > > - Introduce asynchronous threading > > > > This series has the basic infrastructure and few routines like > > mkdir,monod,unlink,readdir,xattr,lstat, etc converted. > > Currently we are working on converting and testing other 9P operations also > > into this model and those patches will follow shortly. > > > > Removing callback functions made some of the patches little lengthy. > > This long patch series adds temporary structs and marshalling code for > each file system operation - I think none of this is necessary. Instead > we can exploit coroutines more: > > The point of coroutines is that you can suspend a thread of control (a > call-stack, not an OS-level thread) and can re-enter it later. We > should make coroutines thread-safe (i.e. work outside of the global > mutex) and then allow switching a coroutine from a QEMU thread to a > worker thread and back again: > > int coroutine_fn v9fs_co_readdir(V9fsState *s, V9fsFidState *fidp, > struct dirent **dent) > { > int ret = 0; > > v9fs_co_run_in_worker({ > errno = 0; > *dent = s->ops->readdir(&s->ctx, fidp->fs.dir); > if (!*dent && errno) { > ret = -errno; > } > }); > return ret; > } > > v9fs_co_readdir() can be called from a QEMU thread. The block of code > inside v9fs_co_run_in_worker() will be executed in a worker thread. > Notice that no marshalling variables is necessary at all; we can use the > function arguments and local variables because this is still the same > function! > > When control reaches the end of the v9fs_co_run_in_worker() block, > execution is resumed in a QEMU thread and the function then returns ret. > It would be incorrect to return inside the v9fs_co_run_in_worker() block > because at that point we're still inside the worker thread. > > Here is how v9fs_co_run_in_worker() does its magic: > > #define v9fs_co_run_in_worker(block) \ > { \ > BH *co_bh; \ > \ > co_bh = qemu_bh_new(co_run_in_worker_bh, qemu_coroutine_self()); \ > qemu_bh_schedule(co_bh); \ > qemu_coroutine_yield(); /* re-entered in worker thread */ \ > qemu_bh_delete(co_bh); \ > \ > block; \ > \ > qemu_coroutine_yield(); /* re-entered in QEMU thread */ \ > } > > void co_run_in_worker_bh(void *opaque) > { > Coroutine *co = opaque; > > g_thread_pool_push(pool, co, NULL); > } > > void worker_thread_fn(gpointer data, gpointer user_data) > { > Coroutine *co = user_data; > char byte = 0; > ssize_t len; > > qemu_coroutine_enter(co, NULL); > > g_async_queue_push(v9fs_pool.completed, co); > do { > len = write(v9fs_pool.wfd, &byte, sizeof(byte)); > } while (len == -1 && errno == EINTR); > } > > void process_req_done(void *arg) > { > Coroutine *co; > char byte; > ssize_t len; > > do { > len = read(v9fs_pool.rfd, &byte, sizeof(byte)); > } while (len == -1 && errno == EINTR); > > while ((co = g_async_queue_try_pop(v9fs_pool.completed)) != NULL) { > qemu_coroutine_enter(co, NULL); > } > } > > I typed this code out in the email, it has not been compiled or tested. > > If you decide to eliminate coroutines entirely in the future and use > worker threads exclusively to process requests, then there are clearly > marked sections in the code: anything inside v9fs_co_run_in_worker() > must be thread-safe already and anything outside it needs to be audited > and made thread-safe. The changes required are smaller than those if > your current patch series was applied. I wanted to mention this point > to show that this doesn't paint virtfs into a corner. > > So where does this leave virtfs? No marshalling is necessary and > blocking operations can be performed inline using > v9fs_co_run_in_worker() blocks. The codebase will be a lot smaller. > > Does this seem reasonable? > Do we really need bottom halfs here ? can't we achieve the same with v9fs_qemu_submit_request() and making the glib thread function callback (request.func())to do qemu_coroutine_enter() like: int v9fs_co_readdir(V9fsState *s, V9fsFidState *fidp, struct dirent **dent) { v9fs_wthread_enter(); s->ops->readdir(&s->ctx, fidp->fs.dir); v9fs_wthread_exit(); } v9fs_worker_thread_enter() { v9fs_qemu_submit_request(v9fs_worker_request); qemu_coroutine_yield(); } v9fs_coroutine_woker_func() { qemu_coroutine_enter(coroutine, NULL); } I also wonder whether additional bottom halfs and additional setcontext/setjmp that we end up with will have a performance impact compared to what we have currently ? -aneesh