Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

> 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