From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35582) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8lvC-0007zT-1T for qemu-devel@nongnu.org; Mon, 12 Aug 2013 02:48:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V8lv2-0004dm-Ph for qemu-devel@nongnu.org; Mon, 12 Aug 2013 02:48:05 -0400 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:35086) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8lv0-0004bb-Ot for qemu-devel@nongnu.org; Mon, 12 Aug 2013 02:47:56 -0400 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 12 Aug 2013 16:40:42 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id BC1843578053 for ; Mon, 12 Aug 2013 16:47:37 +1000 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r7C6lLBB1442168 for ; Mon, 12 Aug 2013 16:47:27 +1000 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r7C6lVaf023687 for ; Mon, 12 Aug 2013 16:47:32 +1000 Message-ID: <520884C0.60208@linux.vnet.ibm.com> Date: Mon, 12 Aug 2013 14:46:24 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <5205B251.2060907@linux.vnet.ibm.com> <5205F3CA.1060009@redhat.com> In-Reply-To: <5205F3CA.1060009@redhat.com> Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , Anthony Liguori , Ping Fan Liu , Stefan Hajnoczi , Michael Roth , qemu-devel > Il 10/08/2013 05:24, Wenchao Xia ha scritto: >> Hi folks, >> I'd like form a series which remove AioContext's concept and >> bind to glib's main loop more closely. Since changed place will be >> a bit much so want to know your opinion before real coding: > > I'm not sure I understand... What does it buy you to split AioContext > this way? First, BhSource and FdSource are needed by block drivers, and > BhSource needs the notifier to interrupt the main loop. Second, > AioContext is _already_ a GSource exactly to integrate closely with > GLib's main loop. Look at the series that introduced AioContext back in > October 2012. The main AioContext is already added as a GSource to the > iothread's main loop; aio_wait is used in dataplane for simplicity, but > it could also use a separate GMainLoop and add AioContext there as a > GSource. > > Paolo > It has two parts: 1) rename AioContext to AioSource. This is my major purpose, which declare it is not a "context" concept, and GMainContext is the entity represent the thread's activity. This can prevent people add wrapper API such as g_main_context_acquire(), g_main_context_prepare(See qcontext_prepare() in QContext patch). As a result, standard glib's main loop API can be exposed, so I can add GMainContext into block layer or any other API layer, instead of custom encapsulation. For example: int bdrv_read(GMainContext *ctx, BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors) In short, it avoid wrapper for GMainContext. I agree AioContext is _already_ a GSource sub class now, there is no difficult to add GMainContext *ctx for AioContext's reason. But I am afraid it becomes not true with more patches comes, since the name is mis-guiding. 2) Break AioSource into FdSource and BhSource. This make custom code less and simpler, one Gsource for one kind of job. It is not necessary but IMHO it will make things clear when add more things into main loop: add a new Gsource sub class, avoid to always have relationship with AioContext. >> changes: >> **before patch: >> typedef struct AioContext { >> GSource source; >> int walking_handlers; >> QemuMutex bh_lock; >> struct QEMUBH *first_bh; >> int walking_bh; >> EventNotifier notifier; >> GArray *pollfds; >> struct ThreadPool *thread_pool; >> } AioContext; >> >> **After patch: >> typedef struct BhSource { >> GSource source; >> QemuMutex bh_lock; >> struct QEMUBH *first_bh; >> int walking_bh; >> } BhSource; >> >> typedef struct FdSource { >> GSource source; >> int walking_handlers; >> EventNotifier notifier; >> GArray *pollfds; >> struct ThreadPool *thread_pool; >> } FdSource; >> >> Benefits: >> Original code have a mix of Gsource and GMainContext's concept, we >> may want to add wrapper functions around GMainContext's functions, such >> as g_main_context_acquire(), g_main_context_prepare(), which brings >> extra effort if you want to form a good and clear API layer. With >> this changes, all qemu's custom code is attached under Gsource, we >> have a clear GMainContext's API layer for event loop, no wrapper is >> needed, and the event's loop API is glib's API, a clear layer let >> me form a library or adding more things. >> >> before: >> qemu's mainloop caller, BH user, fd user >> | >> AioContext >> | >> GMainContext >> >> >> after: >> qemu's mainloop caller >> | BH user fd user >> GmainContext | | >> |--------------------------------|--BhSource | >> |-------------FdSource >> >> >> Note: >> FdSource could be split more into ThreadSource and FdSource, which >> distinguish more. It can be done easily if the change of this series >> is online, when found necessary. >> >> More reasons: >> When I thinking how to bind library code to a thread context, it may >> need to add Context's concept into API of block.c. If I use AioContext, >> there will need a wrapper API to run the event loop. But If I got >> glib's GmainContext, things become simple. > -- Best Regards Wenchao Xia