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

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

[Wenchao Xia]

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:

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

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

[Paolo Bonzini]

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

> 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.

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

[Wenchao Xia]

> 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

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

[Paolo Bonzini]

> 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.

Note that the nested event loops in QEMU are _very_ different from
glib nested event loops.  In QEMU, nested event loops only run block
layer events.  In glib, they run all events.  That's why you need
AioContext.

> 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.

But this is only complicating things work since users rely on both file-
descriptor APIs and bottom half APIs.

> >>    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.

You already have it because AioContext is a GSource.  You do not need
to expose the AioContext, except as a GSource.

Paolo

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

[Michael Roth]

Quoting Paolo Bonzini (2013-08-12 02:30:28)
> > 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.
> 
> Note that the nested event loops in QEMU are _very_ different from
> glib nested event loops.  In QEMU, nested event loops only run block
> layer events.  In glib, they run all events.  That's why you need
> AioContext.

Would it be possible to use glib for our nested loops as well by just
setting a higher priority for the AioContext GSource?

Stefan and I were considering how we could make use of his "drop
ioflush" patches to use a common mechanism to register fd events, but
still allow us to distinguish between AioContext and non-AioContext
for nested loops. I was originally thinking of using prepare() functions
to filter out non-AioContext events, but that requires we implement
on GSource's with that in mind, and non make use of pre-baked ones
like GIOChannel's, and bakes block stuff into every event source
implementation.

Priorities didn't cross my mind though, but it seems pretty
straightfoward...

AioContext could then just be a container of sorts for managing
bottom-halves and AioContext FDs and binding them to the proper
GMainContext/MainLoop, but the underlying GSources could
still be driven by a normal glib-based mainloop, just with a specific
priority in the nested case.

> 
> > 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.
> 
> But this is only complicating things work since users rely on both file-
> descriptor APIs and bottom half APIs.

Taking things a step further, maybe AioContext can stop being a
block-specific construct, but actually be the "QContext" we've
discussed in the past for managing multiple event loops. All
the block stuff would be hidden away in the GSource priority.

For instance,

#ifndef _WIN32

qemu_aio_set_fd_handler(fd, ...):
    aio_set_fd_handler(qemu_aio_context, fd, ..., QEMU_PRIORITY_BLOCK)

qemu_set_fd_handler(fd, ...):
    aio_set_fd_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)

#else

qemu_add_wait_object(fd, ...):
    add_wait_object(qemu_aio_context, fd, ...)

qemu_set_fd_handler(fd, ...):
    set_socket_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)

#endif

qemu_bh_schedule:
    bh_schedule(qemu_aio_context, ...)

etc...

I'll be sending patches this week for moving
add_wait_object/qemu_set_fd_handler to GSources, the non-global ones use
GMainContext * to specify a non-default thread/context, but can be easily
changed, or we can just do aioctx->g_main_context at the call sites.
There's some nice possibilities in using the former though: avoiding
O(n) lookups for stuff like finding the GSource for a particular
event/event type, for instance, by storing pointers to the GSource or
some kind of hashmap lookup. But probably better to discuss that aspect
with some context so I'll try to get those patches out soon.

> 
> > >>    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.
> 
> You already have it because AioContext is a GSource.  You do not need
> to expose the AioContext, except as a GSource.
> 
> Paolo

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

[Wenchao Xia]

于 2013-8-13 1:01, Michael Roth 写道:
> Quoting Paolo Bonzini (2013-08-12 02:30:28)
>>> 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.
>>
>> Note that the nested event loops in QEMU are _very_ different from
>> glib nested event loops.  In QEMU, nested event loops only run block
>> layer events.  In glib, they run all events.  That's why you need
>> AioContext.
>
> Would it be possible to use glib for our nested loops as well by just
> setting a higher priority for the AioContext GSource?
>
> Stefan and I were considering how we could make use of his "drop
> ioflush" patches to use a common mechanism to register fd events, but
> still allow us to distinguish between AioContext and non-AioContext
> for nested loops. I was originally thinking of using prepare() functions
> to filter out non-AioContext events, but that requires we implement
> on GSource's with that in mind, and non make use of pre-baked ones
> like GIOChannel's, and bakes block stuff into every event source
> implementation.
>
   Besides priority, also g_source_set_can_recurse() can help.
   With a deeper think, I found a harder problem:
g_main_context_acquire() and g_main_context_release(). In release,
pending BH/IO call back need to be cleared, but this action can't
be triggered automatically when user call g_main_context_release().
   For the above reason, I tend to think, maybe we should t wrap all of
Glib's mainloop into custom encapsulation, such as QContext, Add the
aio poll logic in q_context_release(). Use QContext * in every caller
to hide GMainContext *, so QContext layer play the role of clear
event loop API.

> Priorities didn't cross my mind though, but it seems pretty
> straightfoward...
>
> AioContext could then just be a container of sorts for managing
> bottom-halves and AioContext FDs and binding them to the proper
> GMainContext/MainLoop, but the underlying GSources could
> still be driven by a normal glib-based mainloop, just with a specific
> priority in the nested case.
>
>>
>>> 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.
>>
>> But this is only complicating things work since users rely on both file-
>> descriptor APIs and bottom half APIs.
>
> Taking things a step further, maybe AioContext can stop being a
> block-specific construct, but actually be the "QContext" we've
> discussed in the past for managing multiple event loops. All
> the block stuff would be hidden away in the GSource priority.
>
> For instance,
>
> #ifndef _WIN32
>
> qemu_aio_set_fd_handler(fd, ...):
>      aio_set_fd_handler(qemu_aio_context, fd, ..., QEMU_PRIORITY_BLOCK)
>
> qemu_set_fd_handler(fd, ...):
>      aio_set_fd_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
>
> #else
>
> qemu_add_wait_object(fd, ...):
>      add_wait_object(qemu_aio_context, fd, ...)
>
> qemu_set_fd_handler(fd, ...):
>      set_socket_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
>
> #endif
>
> qemu_bh_schedule:
>      bh_schedule(qemu_aio_context, ...)
>
> etc...
>
> I'll be sending patches this week for moving
> add_wait_object/qemu_set_fd_handler to GSources, the non-global ones use
> GMainContext * to specify a non-default thread/context, but can be easily
> changed, or we can just do aioctx->g_main_context at the call sites.
> There's some nice possibilities in using the former though: avoiding
> O(n) lookups for stuff like finding the GSource for a particular
> event/event type, for instance, by storing pointers to the GSource or
> some kind of hashmap lookup. But probably better to discuss that aspect
> with some context so I'll try to get those patches out soon.
>
>>
>>>>>     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.
>>
>> You already have it because AioContext is a GSource.  You do not need
>> to expose the AioContext, except as a GSource.
>>
   I think expose GmainContext * or QContext *, is better than
GSource *.

int bdrv_read(GMainContext *ctx,
               BlockDriverState *bs,
               int64_t sector_num,
               uint8_t *buf,
               int nb_sectors)

>> Paolo
>


-- 
Best Regards

Wenchao Xia

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

[Michael Roth]

Quoting Wenchao Xia (2013-08-13 03:44:39)
> 于 2013-8-13 1:01, Michael Roth 写道:
> > Quoting Paolo Bonzini (2013-08-12 02:30:28)
> >>> 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.
> >>
> >> Note that the nested event loops in QEMU are _very_ different from
> >> glib nested event loops.  In QEMU, nested event loops only run block
> >> layer events.  In glib, they run all events.  That's why you need
> >> AioContext.
> >
> > Would it be possible to use glib for our nested loops as well by just
> > setting a higher priority for the AioContext GSource?
> >
> > Stefan and I were considering how we could make use of his "drop
> > ioflush" patches to use a common mechanism to register fd events, but
> > still allow us to distinguish between AioContext and non-AioContext
> > for nested loops. I was originally thinking of using prepare() functions
> > to filter out non-AioContext events, but that requires we implement
> > on GSource's with that in mind, and non make use of pre-baked ones
> > like GIOChannel's, and bakes block stuff into every event source
> > implementation.
> >
>    Besides priority, also g_source_set_can_recurse() can help.
>    With a deeper think, I found a harder problem:
> g_main_context_acquire() and g_main_context_release(). In release,
> pending BH/IO call back need to be cleared, but this action can't
> be triggered automatically when user call g_main_context_release().

I don't understand why this is a requirement, gmctx_acquire/release ensure
that only one thread attempts to iterate the main loop at a time. this
isn't currently an issue in qemu, and if we re-implemented qemu_aio_wait()
to use the same glib interfaces, the tracking of in-flight requests would
be moved to the block layer via Stefan's 'drop io_flush' patches, which
moves that block-specific logic out of the main loop/AioContext GSource
by design. Are there other areas where you see this as a problem?

>    For the above reason, I tend to think, maybe we should t wrap all of
> Glib's mainloop into custom encapsulation, such as QContext, Add the
> aio poll logic in q_context_release(). Use QContext * in every caller
> to hide GMainContext *, so QContext layer play the role of clear
> event loop API.
> 
> > Priorities didn't cross my mind though, but it seems pretty
> > straightfoward...
> >
> > AioContext could then just be a container of sorts for managing
> > bottom-halves and AioContext FDs and binding them to the proper
> > GMainContext/MainLoop, but the underlying GSources could
> > still be driven by a normal glib-based mainloop, just with a specific
> > priority in the nested case.
> >
> >>
> >>> 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.
> >>
> >> But this is only complicating things work since users rely on both file-
> >> descriptor APIs and bottom half APIs.
> >
> > Taking things a step further, maybe AioContext can stop being a
> > block-specific construct, but actually be the "QContext" we've
> > discussed in the past for managing multiple event loops. All
> > the block stuff would be hidden away in the GSource priority.
> >
> > For instance,
> >
> > #ifndef _WIN32
> >
> > qemu_aio_set_fd_handler(fd, ...):
> >      aio_set_fd_handler(qemu_aio_context, fd, ..., QEMU_PRIORITY_BLOCK)
> >
> > qemu_set_fd_handler(fd, ...):
> >      aio_set_fd_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
> >
> > #else
> >
> > qemu_add_wait_object(fd, ...):
> >      add_wait_object(qemu_aio_context, fd, ...)
> >
> > qemu_set_fd_handler(fd, ...):
> >      set_socket_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
> >
> > #endif
> >
> > qemu_bh_schedule:
> >      bh_schedule(qemu_aio_context, ...)
> >
> > etc...
> >
> > I'll be sending patches this week for moving
> > add_wait_object/qemu_set_fd_handler to GSources, the non-global ones use
> > GMainContext * to specify a non-default thread/context, but can be easily
> > changed, or we can just do aioctx->g_main_context at the call sites.
> > There's some nice possibilities in using the former though: avoiding
> > O(n) lookups for stuff like finding the GSource for a particular
> > event/event type, for instance, by storing pointers to the GSource or
> > some kind of hashmap lookup. But probably better to discuss that aspect
> > with some context so I'll try to get those patches out soon.
> >
> >>
> >>>>>     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.
> >>
> >> You already have it because AioContext is a GSource.  You do not need
> >> to expose the AioContext, except as a GSource.
> >>
>    I think expose GmainContext * or QContext *, is better than
> GSource *.
> 
> int bdrv_read(GMainContext *ctx,
>                BlockDriverState *bs,
>                int64_t sector_num,
>                uint8_t *buf,
>                int nb_sectors)
> 
> >> Paolo
> >
> 
> 
> -- 
> Best Regards
> 
> Wenchao Xia

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

[Michael Roth]

Quoting Michael Roth (2013-08-15 10:23:20)
> Quoting Wenchao Xia (2013-08-13 03:44:39)
> > 于 2013-8-13 1:01, Michael Roth 写道:
> > > Quoting Paolo Bonzini (2013-08-12 02:30:28)
> > >>> 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.
> > >>
> > >> Note that the nested event loops in QEMU are _very_ different from
> > >> glib nested event loops.  In QEMU, nested event loops only run block
> > >> layer events.  In glib, they run all events.  That's why you need
> > >> AioContext.
> > >
> > > Would it be possible to use glib for our nested loops as well by just
> > > setting a higher priority for the AioContext GSource?
> > >
> > > Stefan and I were considering how we could make use of his "drop
> > > ioflush" patches to use a common mechanism to register fd events, but
> > > still allow us to distinguish between AioContext and non-AioContext
> > > for nested loops. I was originally thinking of using prepare() functions
> > > to filter out non-AioContext events, but that requires we implement
> > > on GSource's with that in mind, and non make use of pre-baked ones
> > > like GIOChannel's, and bakes block stuff into every event source
> > > implementation.
> > >
> >    Besides priority, also g_source_set_can_recurse() can help.
> >    With a deeper think, I found a harder problem:
> > g_main_context_acquire() and g_main_context_release(). In release,
> > pending BH/IO call back need to be cleared, but this action can't
> > be triggered automatically when user call g_main_context_release().
> 
> I don't understand why this is a requirement, gmctx_acquire/release ensure
> that only one thread attempts to iterate the main loop at a time. this
> isn't currently an issue in qemu, and if we re-implemented qemu_aio_wait()
> to use the same glib interfaces, the tracking of in-flight requests would
> be moved to the block layer via Stefan's 'drop io_flush' patches, which
> moves that block-specific logic out of the main loop/AioContext GSource
> by design. Are there other areas where you see this as a problem?

I think I understand better what you're referring to, you mean that
if qemu_aio_wait was called, and was implementated to essentially call
g_main_context_iterate(), that after, say, 1 iteration, the
iothread/dataplane thread might acquire the main loop and dispatch
block/non-block events between qemu_aio_wait() returned? The simple
approach would be to have qemu_aio_wait() call g_main_context_acquire/release
at the start end of the function, which would ensure that this never
happens.

> 
> >    For the above reason, I tend to think, maybe we should t wrap all of
> > Glib's mainloop into custom encapsulation, such as QContext, Add the
> > aio poll logic in q_context_release(). Use QContext * in every caller
> > to hide GMainContext *, so QContext layer play the role of clear
> > event loop API.
> > 
> > > Priorities didn't cross my mind though, but it seems pretty
> > > straightfoward...
> > >
> > > AioContext could then just be a container of sorts for managing
> > > bottom-halves and AioContext FDs and binding them to the proper
> > > GMainContext/MainLoop, but the underlying GSources could
> > > still be driven by a normal glib-based mainloop, just with a specific
> > > priority in the nested case.
> > >
> > >>
> > >>> 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.
> > >>
> > >> But this is only complicating things work since users rely on both file-
> > >> descriptor APIs and bottom half APIs.
> > >
> > > Taking things a step further, maybe AioContext can stop being a
> > > block-specific construct, but actually be the "QContext" we've
> > > discussed in the past for managing multiple event loops. All
> > > the block stuff would be hidden away in the GSource priority.
> > >
> > > For instance,
> > >
> > > #ifndef _WIN32
> > >
> > > qemu_aio_set_fd_handler(fd, ...):
> > >      aio_set_fd_handler(qemu_aio_context, fd, ..., QEMU_PRIORITY_BLOCK)
> > >
> > > qemu_set_fd_handler(fd, ...):
> > >      aio_set_fd_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
> > >
> > > #else
> > >
> > > qemu_add_wait_object(fd, ...):
> > >      add_wait_object(qemu_aio_context, fd, ...)
> > >
> > > qemu_set_fd_handler(fd, ...):
> > >      set_socket_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
> > >
> > > #endif
> > >
> > > qemu_bh_schedule:
> > >      bh_schedule(qemu_aio_context, ...)
> > >
> > > etc...
> > >
> > > I'll be sending patches this week for moving
> > > add_wait_object/qemu_set_fd_handler to GSources, the non-global ones use
> > > GMainContext * to specify a non-default thread/context, but can be easily
> > > changed, or we can just do aioctx->g_main_context at the call sites.
> > > There's some nice possibilities in using the former though: avoiding
> > > O(n) lookups for stuff like finding the GSource for a particular
> > > event/event type, for instance, by storing pointers to the GSource or
> > > some kind of hashmap lookup. But probably better to discuss that aspect
> > > with some context so I'll try to get those patches out soon.
> > >
> > >>
> > >>>>>     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.
> > >>
> > >> You already have it because AioContext is a GSource.  You do not need
> > >> to expose the AioContext, except as a GSource.
> > >>
> >    I think expose GmainContext * or QContext *, is better than
> > GSource *.
> > 
> > int bdrv_read(GMainContext *ctx,
> >                BlockDriverState *bs,
> >                int64_t sector_num,
> >                uint8_t *buf,
> >                int nb_sectors)
> > 
> > >> Paolo
> > >
> > 
> > 
> > -- 
> > Best Regards
> > 
> > Wenchao Xia

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

[Wenchao Xia]

于 2013-8-16 0:32, Michael Roth 写道:
> Quoting Michael Roth (2013-08-15 10:23:20)
>> Quoting Wenchao Xia (2013-08-13 03:44:39)
>>> 于 2013-8-13 1:01, Michael Roth 写道:
>>>> Quoting Paolo Bonzini (2013-08-12 02:30:28)
>>>>>> 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.
>>>>>
>>>>> Note that the nested event loops in QEMU are _very_ different from
>>>>> glib nested event loops.  In QEMU, nested event loops only run block
>>>>> layer events.  In glib, they run all events.  That's why you need
>>>>> AioContext.
>>>>
>>>> Would it be possible to use glib for our nested loops as well by just
>>>> setting a higher priority for the AioContext GSource?
>>>>
>>>> Stefan and I were considering how we could make use of his "drop
>>>> ioflush" patches to use a common mechanism to register fd events, but
>>>> still allow us to distinguish between AioContext and non-AioContext
>>>> for nested loops. I was originally thinking of using prepare() functions
>>>> to filter out non-AioContext events, but that requires we implement
>>>> on GSource's with that in mind, and non make use of pre-baked ones
>>>> like GIOChannel's, and bakes block stuff into every event source
>>>> implementation.
>>>>
>>>     Besides priority, also g_source_set_can_recurse() can help.
>>>     With a deeper think, I found a harder problem:
>>> g_main_context_acquire() and g_main_context_release(). In release,
>>> pending BH/IO call back need to be cleared, but this action can't
>>> be triggered automatically when user call g_main_context_release().
>>
>> I don't understand why this is a requirement, gmctx_acquire/release ensure
>> that only one thread attempts to iterate the main loop at a time. this
>> isn't currently an issue in qemu, and if we re-implemented qemu_aio_wait()
>> to use the same glib interfaces, the tracking of in-flight requests would
>> be moved to the block layer via Stefan's 'drop io_flush' patches, which
>> moves that block-specific logic out of the main loop/AioContext GSource
>> by design. Are there other areas where you see this as a problem?
>
> I think I understand better what you're referring to, you mean that
> if qemu_aio_wait was called, and was implementated to essentially call
> g_main_context_iterate(), that after, say, 1 iteration, the
> iothread/dataplane thread might acquire the main loop and dispatch
> block/non-block events between qemu_aio_wait() returned? The simple
> approach would be to have qemu_aio_wait() call g_main_context_acquire/release
> at the start end of the function, which would ensure that this never
> happens.
>
   qemu_aio_wait() is relative simple to resolve by 
g_main_context_acquire(), but also shows additional code needed
for a thread switch:
(guess following is the plan to implement qemu_aio_wait())
qemu_aio_wait(GMainContext *ctx)
{
     return g_main_context(ctx, PRI_BLOCK);
}
at caller:
{
     while (qemu_aio_wait(ctx) {
         ;
     }
     g_main_context_release(ctx);
}
   The above code shows that, in *ctx switch operation, there is
more than glib's own event loop API envolved, qemu_aio_wait(). So
it referenced to a question: what data structure
should be used to represent context concept and control the thread
switching behavior?  It is better to have a clear layer, GMainContext or
GlibQContext, instead of GMainContext plus custom function. The caller
reference to at least two: nested user block layer, flat user above
block layer.
   In my opinion, this problem is brought by Gsource AioContext, Take
the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
an example, there are aync following operations involved for AioContext:
1 the bdrv_cb() will be executed in bdrv_co_em_bh().
2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
3 aio_worker() will be executed in worker_thread().
   Operation 2 and 3 are tracked by block layer's queue after Stefan's
dropping io_flush() series.
   Now if we stick to GMainContext to represent context concept,
then when thread B want to aquire GMainContext used by thread A,
all works setupped by A should be finished before B aquire it,
otherwise B will execute some function supposed to work in A. The
work refers to the three kind of aync functions above.
   For this issue, we can solve it by different means:
1 the event loop API doesn't guarentee work setupped by thread A
will always be finished in A. This put a limitation to caller to
consider thread switching. I talked on IRC with Stefan, and thinks
it is possible for the nested user block layer, but I still want to
avoid this to the flat user above block layer.
2 ask caller to finish all pending operations.
   2.1 glib GMainContext API plus custom API such as aio_wait(). This is
bad that detail under GMainContext is exposed to caller. Since
operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
is called in thread setupped it, 1 need to be added in the track
queue, or in the call chain of aio_wait(), check the queue plus check
operation 1. Perhaps add a custom function ask caller to call as
context_work_flush()?
   2.2 Use GlibQContext API. In this way all operation is hidden,
in whose release() the pending work will be finished. In this way
a clear layer represent event loop can be exposed.

   A clear layer represent event loop will make caller code clean,
especially helpful when try to expose block layer with AIO as a
public library.
   Personally I hope not to use AioContext as context concept, add
iterate APIs around it will be tricky, since actually it is a GSource,
it will bring troubles to integrate with glib's event loop from my
intuition. So I hope to rename AioContext to AioSource, or break it
as a common case in QContext. Then I got bdrv_read(GlibQContext ctx*).

   With a talk with Stefan, I plan to read gstream doc to see how it
deal with such problem.

>>
>>>     For the above reason, I tend to think, maybe we should t wrap all of
>>> Glib's mainloop into custom encapsulation, such as QContext, Add the
>>> aio poll logic in q_context_release(). Use QContext * in every caller
>>> to hide GMainContext *, so QContext layer play the role of clear
>>> event loop API.
>>>
>>>> Priorities didn't cross my mind though, but it seems pretty
>>>> straightfoward...
>>>>
>>>> AioContext could then just be a container of sorts for managing
>>>> bottom-halves and AioContext FDs and binding them to the proper
>>>> GMainContext/MainLoop, but the underlying GSources could
>>>> still be driven by a normal glib-based mainloop, just with a specific
>>>> priority in the nested case.
>>>>
>>>>>
>>>>>> 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.
>>>>>
>>>>> But this is only complicating things work since users rely on both file-
>>>>> descriptor APIs and bottom half APIs.
>>>>
>>>> Taking things a step further, maybe AioContext can stop being a
>>>> block-specific construct, but actually be the "QContext" we've
>>>> discussed in the past for managing multiple event loops. All
>>>> the block stuff would be hidden away in the GSource priority.
>>>>
>>>> For instance,
>>>>
>>>> #ifndef _WIN32
>>>>
>>>> qemu_aio_set_fd_handler(fd, ...):
>>>>       aio_set_fd_handler(qemu_aio_context, fd, ..., QEMU_PRIORITY_BLOCK)
>>>>
>>>> qemu_set_fd_handler(fd, ...):
>>>>       aio_set_fd_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
>>>>
>>>> #else
>>>>
>>>> qemu_add_wait_object(fd, ...):
>>>>       add_wait_object(qemu_aio_context, fd, ...)
>>>>
>>>> qemu_set_fd_handler(fd, ...):
>>>>       set_socket_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT)
>>>>
>>>> #endif
>>>>
>>>> qemu_bh_schedule:
>>>>       bh_schedule(qemu_aio_context, ...)
>>>>
>>>> etc...
>>>>
>>>> I'll be sending patches this week for moving
>>>> add_wait_object/qemu_set_fd_handler to GSources, the non-global ones use
>>>> GMainContext * to specify a non-default thread/context, but can be easily
>>>> changed, or we can just do aioctx->g_main_context at the call sites.
>>>> There's some nice possibilities in using the former though: avoiding
>>>> O(n) lookups for stuff like finding the GSource for a particular
>>>> event/event type, for instance, by storing pointers to the GSource or
>>>> some kind of hashmap lookup. But probably better to discuss that aspect
>>>> with some context so I'll try to get those patches out soon.
>>>>
>>>>>
>>>>>>>>      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.
>>>>>
>>>>> You already have it because AioContext is a GSource.  You do not need
>>>>> to expose the AioContext, except as a GSource.
>>>>>
>>>     I think expose GmainContext * or QContext *, is better than
>>> GSource *.
>>>
>>> int bdrv_read(GMainContext *ctx,
>>>                 BlockDriverState *bs,
>>>                 int64_t sector_num,
>>>                 uint8_t *buf,
>>>                 int nb_sectors)
>>>
>>>>> Paolo
>>>>
>>>
>>>
>>> --
>>> Best Regards
>>>
>>> Wenchao Xia
>


-- 
Best Regards

Wenchao Xia

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

[Wenchao Xia]

于 2013-8-16 15:15, Wenchao Xia 写道:
> 于 2013-8-16 0:32, Michael Roth 写道:
>> Quoting Michael Roth (2013-08-15 10:23:20)
>>> Quoting Wenchao Xia (2013-08-13 03:44:39)
>>>> 于 2013-8-13 1:01, Michael Roth 写道:
>>>>> Quoting Paolo Bonzini (2013-08-12 02:30:28)
>>>>>>> 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.
>>>>>>
>>>>>> Note that the nested event loops in QEMU are _very_ different from
>>>>>> glib nested event loops.  In QEMU, nested event loops only run block
>>>>>> layer events.  In glib, they run all events.  That's why you need
>>>>>> AioContext.
>>>>>
>>>>> Would it be possible to use glib for our nested loops as well by just
>>>>> setting a higher priority for the AioContext GSource?
>>>>>
>>>>> Stefan and I were considering how we could make use of his "drop
>>>>> ioflush" patches to use a common mechanism to register fd events, but
>>>>> still allow us to distinguish between AioContext and non-AioContext
>>>>> for nested loops. I was originally thinking of using prepare()
>>>>> functions
>>>>> to filter out non-AioContext events, but that requires we implement
>>>>> on GSource's with that in mind, and non make use of pre-baked ones
>>>>> like GIOChannel's, and bakes block stuff into every event source
>>>>> implementation.
>>>>>
>>>>     Besides priority, also g_source_set_can_recurse() can help.
>>>>     With a deeper think, I found a harder problem:
>>>> g_main_context_acquire() and g_main_context_release(). In release,
>>>> pending BH/IO call back need to be cleared, but this action can't
>>>> be triggered automatically when user call g_main_context_release().
>>>
>>> I don't understand why this is a requirement, gmctx_acquire/release
>>> ensure
>>> that only one thread attempts to iterate the main loop at a time. this
>>> isn't currently an issue in qemu, and if we re-implemented
>>> qemu_aio_wait()
>>> to use the same glib interfaces, the tracking of in-flight requests
>>> would
>>> be moved to the block layer via Stefan's 'drop io_flush' patches, which
>>> moves that block-specific logic out of the main loop/AioContext GSource
>>> by design. Are there other areas where you see this as a problem?
>>
>> I think I understand better what you're referring to, you mean that
>> if qemu_aio_wait was called, and was implementated to essentially call
>> g_main_context_iterate(), that after, say, 1 iteration, the
>> iothread/dataplane thread might acquire the main loop and dispatch
>> block/non-block events between qemu_aio_wait() returned? The simple
>> approach would be to have qemu_aio_wait() call
>> g_main_context_acquire/release
>> at the start end of the function, which would ensure that this never
>> happens.
>>
>    qemu_aio_wait() is relative simple to resolve by
> g_main_context_acquire(), but also shows additional code needed
> for a thread switch:
> (guess following is the plan to implement qemu_aio_wait())
> qemu_aio_wait(GMainContext *ctx)
> {
>      return g_main_context(ctx, PRI_BLOCK);
> }
> at caller:
> {
>      while (qemu_aio_wait(ctx) {
>          ;
>      }
>      g_main_context_release(ctx);
> }
>    The above code shows that, in *ctx switch operation, there is
> more than glib's own event loop API envolved, qemu_aio_wait(). So
> it referenced to a question: what data structure
> should be used to represent context concept and control the thread
> switching behavior?  It is better to have a clear layer, GMainContext or
> GlibQContext, instead of GMainContext plus custom function. The caller
> reference to at least two: nested user block layer, flat user above
> block layer.
>    In my opinion, this problem is brought by Gsource AioContext, Take
> the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
> an example, there are aync following operations involved for AioContext:
> 1 the bdrv_cb() will be executed in bdrv_co_em_bh().
> 2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
> 3 aio_worker() will be executed in worker_thread().
>    Operation 2 and 3 are tracked by block layer's queue after Stefan's
> dropping io_flush() series.
>    Now if we stick to GMainContext to represent context concept,
> then when thread B want to aquire GMainContext used by thread A,
> all works setupped by A should be finished before B aquire it,
> otherwise B will execute some function supposed to work in A. The
> work refers to the three kind of aync functions above.
>    For this issue, we can solve it by different means:
> 1 the event loop API doesn't guarentee work setupped by thread A
> will always be finished in A. This put a limitation to caller to
> consider thread switching. I talked on IRC with Stefan, and thinks
> it is possible for the nested user block layer, but I still want to
> avoid this to the flat user above block layer.
> 2 ask caller to finish all pending operations.
>    2.1 glib GMainContext API plus custom API such as aio_wait(). This is
> bad that detail under GMainContext is exposed to caller. Since
> operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
> is called in thread setupped it, 1 need to be added in the track
> queue, or in the call chain of aio_wait(), check the queue plus check
> operation 1. Perhaps add a custom function ask caller to call as
> context_work_flush()?
   If a well named API do the flush work present, using Glib API plus
it seems also OK, and can avoid wrapper. I guess
bdrv_drain_all(GMainContext *ctx, ...) can do it.


>    2.2 Use GlibQContext API. In this way all operation is hidden,
> in whose release() the pending work will be finished. In this way
> a clear layer represent event loop can be exposed.
>
>    A clear layer represent event loop will make caller code clean,
> especially helpful when try to expose block layer with AIO as a
> public library.
>    Personally I hope not to use AioContext as context concept, add
> iterate APIs around it will be tricky, since actually it is a GSource,
> it will bring troubles to integrate with glib's event loop from my
> intuition. So I hope to rename AioContext to AioSource, or break it
> as a common case in QContext. Then I got bdrv_read(GlibQContext ctx*).
>
>    With a talk with Stefan, I plan to read gstream doc to see how it
> deal with such problem.
>
>>>
>>>>     For the above reason, I tend to think, maybe we should t wrap
>>>> all of
>>>> Glib's mainloop into custom encapsulation, such as QContext, Add the
>>>> aio poll logic in q_context_release(). Use QContext * in every caller
>>>> to hide GMainContext *, so QContext layer play the role of clear
>>>> event loop API.
>>>>
>>>>> Priorities didn't cross my mind though, but it seems pretty
>>>>> straightfoward...
>>>>>
>>>>> AioContext could then just be a container of sorts for managing
>>>>> bottom-halves and AioContext FDs and binding them to the proper
>>>>> GMainContext/MainLoop, but the underlying GSources could
>>>>> still be driven by a normal glib-based mainloop, just with a specific
>>>>> priority in the nested case.
>>>>>
>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> But this is only complicating things work since users rely on both
>>>>>> file-
>>>>>> descriptor APIs and bottom half APIs.
>>>>>
>>>>> Taking things a step further, maybe AioContext can stop being a
>>>>> block-specific construct, but actually be the "QContext" we've
>>>>> discussed in the past for managing multiple event loops. All
>>>>> the block stuff would be hidden away in the GSource priority.
>>>>>
>>>>> For instance,
>>>>>
>>>>> #ifndef _WIN32
>>>>>
>>>>> qemu_aio_set_fd_handler(fd, ...):
>>>>>       aio_set_fd_handler(qemu_aio_context, fd, ...,
>>>>> QEMU_PRIORITY_BLOCK)
>>>>>
>>>>> qemu_set_fd_handler(fd, ...):
>>>>>       aio_set_fd_handler(qemu_aio_context, fd, ...,
>>>>> G_PRIORITY_DEFAULT)
>>>>>
>>>>> #else
>>>>>
>>>>> qemu_add_wait_object(fd, ...):
>>>>>       add_wait_object(qemu_aio_context, fd, ...)
>>>>>
>>>>> qemu_set_fd_handler(fd, ...):
>>>>>       set_socket_handler(qemu_aio_context, fd, ...,
>>>>> G_PRIORITY_DEFAULT)
>>>>>
>>>>> #endif
>>>>>
>>>>> qemu_bh_schedule:
>>>>>       bh_schedule(qemu_aio_context, ...)
>>>>>
>>>>> etc...
>>>>>
>>>>> I'll be sending patches this week for moving
>>>>> add_wait_object/qemu_set_fd_handler to GSources, the non-global
>>>>> ones use
>>>>> GMainContext * to specify a non-default thread/context, but can be
>>>>> easily
>>>>> changed, or we can just do aioctx->g_main_context at the call sites.
>>>>> There's some nice possibilities in using the former though: avoiding
>>>>> O(n) lookups for stuff like finding the GSource for a particular
>>>>> event/event type, for instance, by storing pointers to the GSource or
>>>>> some kind of hashmap lookup. But probably better to discuss that
>>>>> aspect
>>>>> with some context so I'll try to get those patches out soon.
>>>>>
>>>>>>
>>>>>>>>>      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.
>>>>>>
>>>>>> You already have it because AioContext is a GSource.  You do not need
>>>>>> to expose the AioContext, except as a GSource.
>>>>>>
>>>>     I think expose GmainContext * or QContext *, is better than
>>>> GSource *.
>>>>
>>>> int bdrv_read(GMainContext *ctx,
>>>>                 BlockDriverState *bs,
>>>>                 int64_t sector_num,
>>>>                 uint8_t *buf,
>>>>                 int nb_sectors)
>>>>
>>>>>> Paolo
>>>>>
>>>>
>>>>
>>>> --
>>>> Best Regards
>>>>
>>>> Wenchao Xia
>>
>
>


-- 
Best Regards

Wenchao Xia

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

[Wenchao Xia]

于 2013-8-16 16:12, Wenchao Xia 写道:
> 于 2013-8-16 15:15, Wenchao Xia 写道:
>> 于 2013-8-16 0:32, Michael Roth 写道:
>>> Quoting Michael Roth (2013-08-15 10:23:20)
>>>> Quoting Wenchao Xia (2013-08-13 03:44:39)
>>>>> 于 2013-8-13 1:01, Michael Roth 写道:
>>>>>> Quoting Paolo Bonzini (2013-08-12 02:30:28)
>>>>>>>> 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.
>>>>>>>
>>>>>>> Note that the nested event loops in QEMU are _very_ different from
>>>>>>> glib nested event loops.  In QEMU, nested event loops only run block
>>>>>>> layer events.  In glib, they run all events.  That's why you need
>>>>>>> AioContext.
>>>>>>
>>>>>> Would it be possible to use glib for our nested loops as well by just
>>>>>> setting a higher priority for the AioContext GSource?
>>>>>>
>>>>>> Stefan and I were considering how we could make use of his "drop
>>>>>> ioflush" patches to use a common mechanism to register fd events, but
>>>>>> still allow us to distinguish between AioContext and non-AioContext
>>>>>> for nested loops. I was originally thinking of using prepare()
>>>>>> functions
>>>>>> to filter out non-AioContext events, but that requires we implement
>>>>>> on GSource's with that in mind, and non make use of pre-baked ones
>>>>>> like GIOChannel's, and bakes block stuff into every event source
>>>>>> implementation.
>>>>>>
>>>>>     Besides priority, also g_source_set_can_recurse() can help.
>>>>>     With a deeper think, I found a harder problem:
>>>>> g_main_context_acquire() and g_main_context_release(). In release,
>>>>> pending BH/IO call back need to be cleared, but this action can't
>>>>> be triggered automatically when user call g_main_context_release().
>>>>
>>>> I don't understand why this is a requirement, gmctx_acquire/release
>>>> ensure
>>>> that only one thread attempts to iterate the main loop at a time. this
>>>> isn't currently an issue in qemu, and if we re-implemented
>>>> qemu_aio_wait()
>>>> to use the same glib interfaces, the tracking of in-flight requests
>>>> would
>>>> be moved to the block layer via Stefan's 'drop io_flush' patches, which
>>>> moves that block-specific logic out of the main loop/AioContext GSource
>>>> by design. Are there other areas where you see this as a problem?
>>>
>>> I think I understand better what you're referring to, you mean that
>>> if qemu_aio_wait was called, and was implementated to essentially call
>>> g_main_context_iterate(), that after, say, 1 iteration, the
>>> iothread/dataplane thread might acquire the main loop and dispatch
>>> block/non-block events between qemu_aio_wait() returned? The simple
>>> approach would be to have qemu_aio_wait() call
>>> g_main_context_acquire/release
>>> at the start end of the function, which would ensure that this never
>>> happens.
>>>
>>    qemu_aio_wait() is relative simple to resolve by
>> g_main_context_acquire(), but also shows additional code needed
>> for a thread switch:
>> (guess following is the plan to implement qemu_aio_wait())
>> qemu_aio_wait(GMainContext *ctx)
>> {
>>      return g_main_context(ctx, PRI_BLOCK);
>> }
>> at caller:
>> {
>>      while (qemu_aio_wait(ctx) {
>>          ;
>>      }
>>      g_main_context_release(ctx);
>> }
>>    The above code shows that, in *ctx switch operation, there is
>> more than glib's own event loop API envolved, qemu_aio_wait(). So
>> it referenced to a question: what data structure
>> should be used to represent context concept and control the thread
>> switching behavior?  It is better to have a clear layer, GMainContext or
>> GlibQContext, instead of GMainContext plus custom function. The caller
>> reference to at least two: nested user block layer, flat user above
>> block layer.
>>    In my opinion, this problem is brought by Gsource AioContext, Take
>> the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
>> an example, there are aync following operations involved for AioContext:
>> 1 the bdrv_cb() will be executed in bdrv_co_em_bh().
>> 2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
>> 3 aio_worker() will be executed in worker_thread().
>>    Operation 2 and 3 are tracked by block layer's queue after Stefan's
>> dropping io_flush() series.
>>    Now if we stick to GMainContext to represent context concept,
>> then when thread B want to aquire GMainContext used by thread A,
>> all works setupped by A should be finished before B aquire it,
>> otherwise B will execute some function supposed to work in A. The
>> work refers to the three kind of aync functions above.
>>    For this issue, we can solve it by different means:
>> 1 the event loop API doesn't guarentee work setupped by thread A
>> will always be finished in A. This put a limitation to caller to
>> consider thread switching. I talked on IRC with Stefan, and thinks
>> it is possible for the nested user block layer, but I still want to
>> avoid this to the flat user above block layer.
>> 2 ask caller to finish all pending operations.
>>    2.1 glib GMainContext API plus custom API such as aio_wait(). This is
>> bad that detail under GMainContext is exposed to caller. Since
>> operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
>> is called in thread setupped it, 1 need to be added in the track
>> queue, or in the call chain of aio_wait(), check the queue plus check
>> operation 1. Perhaps add a custom function ask caller to call as
>> context_work_flush()?
>    If a well named API do the flush work present, using Glib API plus
> it seems also OK, and can avoid wrapper. I guess
> bdrv_drain_all(GMainContext *ctx, ...) can do it.
>
   I haven't found a good answer in gstream, but want to show
some idea from my understanding.

Following is a brief picture of the current event loop in qemu,
Alex's timer for AioContext is also drawn here:

                                               ========================
                                               || I/O thread in vl.c ||
                                               ========================
                                                           |
                                                  run loop |
                                                           |
====================                                      |
||    other       || qemu_set_fd_handler2()      =====================
||                ||-----------------------------||   Main_loop     ||
||(vnc, migration)||                   |         =====================
====================                   |          GLib    |
                                        |    event loop API|
                   qemu_set_fd_handler()|                  |
                        -----------------          ====================
                        |                          ||  GMainContext  ||
                        |                          ====================
==========             |   (should it be removed?)        |
||  hw  ||--------------------------------------          |GSouce
==========                      |               |         |Attach
                                 |  main_loop_tlg|         |
                    qemu_bh_***()|               |         |
                                 |               |         |
                           ======|===============|=======================
                           ||    |               |                     ||
===========               ||  ======   ==================   =======   ||
|| block ||---------------||  | BH |   | TimerListGroup |   | Fd  |   ||
===========  qemu_bh_***()||  ======   ==================   =======   ||
            qemu_aio_wait()||                                          ||
  qemu_aio_set_fd_handler()||  AioContext                              ||
                           || (block layer's event collection)         ||
                            =============================================


   The main issue here is that components are tightly bind together and
no clear layer represent the thread and event loop API. Block and hw
code are inter acting with AioContext, so both GMainContext and
AioContext are playing the role. I hope to form a library for block,
So need to pick up one to provide event loop, the choice seems to be:
1 GMainContext.
2 AioContext.
3 Encapsulation, such as GlibQContext.

   1) and 2) would not be perfect since non standard glib event loop will
be exposed, 3) will shows a unified interface similar to glib main loop,
but more code adjust. After some thinking, I guess AioContext is the
easiest way, which represent the block's own event loop, and give up
using glib event loop at this level, just add custom API as
block_iterate(). Briefly, bdrv_read will becomes:
int bdrv_read(AioContext *ctx, ....);

   Dear maintainers, what do you think?

>
>>    2.2 Use GlibQContext API. In this way all operation is hidden,
>> in whose release() the pending work will be finished. In this way
>> a clear layer represent event loop can be exposed.
>>
>>    A clear layer represent event loop will make caller code clean,
>> especially helpful when try to expose block layer with AIO as a
>> public library.
>>    Personally I hope not to use AioContext as context concept, add
>> iterate APIs around it will be tricky, since actually it is a GSource,
>> it will bring troubles to integrate with glib's event loop from my
>> intuition. So I hope to rename AioContext to AioSource, or break it
>> as a common case in QContext. Then I got bdrv_read(GlibQContext ctx*).
>>
>>    With a talk with Stefan, I plan to read gstream doc to see how it
>> deal with such problem.
>>
>>>>
>>>>>     For the above reason, I tend to think, maybe we should t wrap
>>>>> all of
>>>>> Glib's mainloop into custom encapsulation, such as QContext, Add the
>>>>> aio poll logic in q_context_release(). Use QContext * in every caller
>>>>> to hide GMainContext *, so QContext layer play the role of clear
>>>>> event loop API.
>>>>>
>>>>>> Priorities didn't cross my mind though, but it seems pretty
>>>>>> straightfoward...
>>>>>>
>>>>>> AioContext could then just be a container of sorts for managing
>>>>>> bottom-halves and AioContext FDs and binding them to the proper
>>>>>> GMainContext/MainLoop, but the underlying GSources could
>>>>>> still be driven by a normal glib-based mainloop, just with a specific
>>>>>> priority in the nested case.
>>>>>>
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> But this is only complicating things work since users rely on both
>>>>>>> file-
>>>>>>> descriptor APIs and bottom half APIs.
>>>>>>
>>>>>> Taking things a step further, maybe AioContext can stop being a
>>>>>> block-specific construct, but actually be the "QContext" we've
>>>>>> discussed in the past for managing multiple event loops. All
>>>>>> the block stuff would be hidden away in the GSource priority.
>>>>>>
>>>>>> For instance,
>>>>>>
>>>>>> #ifndef _WIN32
>>>>>>
>>>>>> qemu_aio_set_fd_handler(fd, ...):
>>>>>>       aio_set_fd_handler(qemu_aio_context, fd, ...,
>>>>>> QEMU_PRIORITY_BLOCK)
>>>>>>
>>>>>> qemu_set_fd_handler(fd, ...):
>>>>>>       aio_set_fd_handler(qemu_aio_context, fd, ...,
>>>>>> G_PRIORITY_DEFAULT)
>>>>>>
>>>>>> #else
>>>>>>
>>>>>> qemu_add_wait_object(fd, ...):
>>>>>>       add_wait_object(qemu_aio_context, fd, ...)
>>>>>>
>>>>>> qemu_set_fd_handler(fd, ...):
>>>>>>       set_socket_handler(qemu_aio_context, fd, ...,
>>>>>> G_PRIORITY_DEFAULT)
>>>>>>
>>>>>> #endif
>>>>>>
>>>>>> qemu_bh_schedule:
>>>>>>       bh_schedule(qemu_aio_context, ...)
>>>>>>
>>>>>> etc...
>>>>>>
>>>>>> I'll be sending patches this week for moving
>>>>>> add_wait_object/qemu_set_fd_handler to GSources, the non-global
>>>>>> ones use
>>>>>> GMainContext * to specify a non-default thread/context, but can be
>>>>>> easily
>>>>>> changed, or we can just do aioctx->g_main_context at the call sites.
>>>>>> There's some nice possibilities in using the former though: avoiding
>>>>>> O(n) lookups for stuff like finding the GSource for a particular
>>>>>> event/event type, for instance, by storing pointers to the GSource or
>>>>>> some kind of hashmap lookup. But probably better to discuss that
>>>>>> aspect
>>>>>> with some context so I'll try to get those patches out soon.
>>>>>>
>>>>>>>
>>>>>>>>>>      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.
>>>>>>>
>>>>>>> You already have it because AioContext is a GSource.  You do not
>>>>>>> need
>>>>>>> to expose the AioContext, except as a GSource.
>>>>>>>
>>>>>     I think expose GmainContext * or QContext *, is better than
>>>>> GSource *.
>>>>>
>>>>> int bdrv_read(GMainContext *ctx,
>>>>>                 BlockDriverState *bs,
>>>>>                 int64_t sector_num,
>>>>>                 uint8_t *buf,
>>>>>                 int nb_sectors)
>>>>>
>>>>>>> Paolo
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best Regards
>>>>>
>>>>> Wenchao Xia
>>>
>>
>>
>
>


-- 
Best Regards

Wenchao Xia

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

[Alex Bligh]

On 20 Aug 2013, at 10:59, Wenchao Xia wrote:

>  The main issue here is that components are tightly bind together and
> no clear layer represent the thread and event loop API. Block and hw
> code are inter acting with AioContext, so both GMainContext and
> AioContext are playing the role. I hope to form a library for block,
> So need to pick up one to provide event loop, the choice seems to be:
> 1 GMainContext.
> 2 AioContext.
> 3 Encapsulation, such as GlibQContext.
> 
>  1) and 2) would not be perfect since non standard glib event loop will
> be exposed, 3) will shows a unified interface similar to glib main loop,
> but more code adjust. After some thinking, I guess AioContext is the
> easiest way, which represent the block's own event loop, and give up
> using glib event loop at this level, just add custom API as
> block_iterate(). Briefly, bdrv_read will becomes:
> int bdrv_read(AioContext *ctx, ....);

My 2p: I'd rather lose the glib classes for contexts. Partly because
of the lack of nanosecond support (I verified that glib DOES
play around with the timing values rather than pass them
in an opaque manner), but mostly because (as illustrated
by a different thread today) the interface is a bit opaque towards
the gsource stuff. We could easily have a less general purpose
library which would be simpler and more predictable.

I guess that leaves either (2), or an option (4) - we use our
own encapsulation of AioContext - a sort of AioContext
superclass which would handle the loop stuff but need
not be modelled on block I/O. This could remain modelled on the
glib interface (as AioContext is) if we want.

-- 
Alex Bligh

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

[Stefan Hajnoczi]

On Tue, Aug 20, 2013 at 05:59:58PM +0800, Wenchao Xia wrote:
> 于 2013-8-16 16:12, Wenchao Xia 写道:
> >于 2013-8-16 15:15, Wenchao Xia 写道:
> >>于 2013-8-16 0:32, Michael Roth 写道:
> >>>Quoting Michael Roth (2013-08-15 10:23:20)
> >>>>Quoting Wenchao Xia (2013-08-13 03:44:39)
> >>>>>于 2013-8-13 1:01, Michael Roth 写道:
> >>>>>>Quoting Paolo Bonzini (2013-08-12 02:30:28)
> >>>>>>>>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.
> >>>>>>>
> >>>>>>>Note that the nested event loops in QEMU are _very_ different from
> >>>>>>>glib nested event loops.  In QEMU, nested event loops only run block
> >>>>>>>layer events.  In glib, they run all events.  That's why you need
> >>>>>>>AioContext.
> >>>>>>
> >>>>>>Would it be possible to use glib for our nested loops as well by just
> >>>>>>setting a higher priority for the AioContext GSource?
> >>>>>>
> >>>>>>Stefan and I were considering how we could make use of his "drop
> >>>>>>ioflush" patches to use a common mechanism to register fd events, but
> >>>>>>still allow us to distinguish between AioContext and non-AioContext
> >>>>>>for nested loops. I was originally thinking of using prepare()
> >>>>>>functions
> >>>>>>to filter out non-AioContext events, but that requires we implement
> >>>>>>on GSource's with that in mind, and non make use of pre-baked ones
> >>>>>>like GIOChannel's, and bakes block stuff into every event source
> >>>>>>implementation.
> >>>>>>
> >>>>>    Besides priority, also g_source_set_can_recurse() can help.
> >>>>>    With a deeper think, I found a harder problem:
> >>>>>g_main_context_acquire() and g_main_context_release(). In release,
> >>>>>pending BH/IO call back need to be cleared, but this action can't
> >>>>>be triggered automatically when user call g_main_context_release().
> >>>>
> >>>>I don't understand why this is a requirement, gmctx_acquire/release
> >>>>ensure
> >>>>that only one thread attempts to iterate the main loop at a time. this
> >>>>isn't currently an issue in qemu, and if we re-implemented
> >>>>qemu_aio_wait()
> >>>>to use the same glib interfaces, the tracking of in-flight requests
> >>>>would
> >>>>be moved to the block layer via Stefan's 'drop io_flush' patches, which
> >>>>moves that block-specific logic out of the main loop/AioContext GSource
> >>>>by design. Are there other areas where you see this as a problem?
> >>>
> >>>I think I understand better what you're referring to, you mean that
> >>>if qemu_aio_wait was called, and was implementated to essentially call
> >>>g_main_context_iterate(), that after, say, 1 iteration, the
> >>>iothread/dataplane thread might acquire the main loop and dispatch
> >>>block/non-block events between qemu_aio_wait() returned? The simple
> >>>approach would be to have qemu_aio_wait() call
> >>>g_main_context_acquire/release
> >>>at the start end of the function, which would ensure that this never
> >>>happens.
> >>>
> >>   qemu_aio_wait() is relative simple to resolve by
> >>g_main_context_acquire(), but also shows additional code needed
> >>for a thread switch:
> >>(guess following is the plan to implement qemu_aio_wait())
> >>qemu_aio_wait(GMainContext *ctx)
> >>{
> >>     return g_main_context(ctx, PRI_BLOCK);
> >>}
> >>at caller:
> >>{
> >>     while (qemu_aio_wait(ctx) {
> >>         ;
> >>     }
> >>     g_main_context_release(ctx);
> >>}
> >>   The above code shows that, in *ctx switch operation, there is
> >>more than glib's own event loop API envolved, qemu_aio_wait(). So
> >>it referenced to a question: what data structure
> >>should be used to represent context concept and control the thread
> >>switching behavior?  It is better to have a clear layer, GMainContext or
> >>GlibQContext, instead of GMainContext plus custom function. The caller
> >>reference to at least two: nested user block layer, flat user above
> >>block layer.
> >>   In my opinion, this problem is brought by Gsource AioContext, Take
> >>the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
> >>an example, there are aync following operations involved for AioContext:
> >>1 the bdrv_cb() will be executed in bdrv_co_em_bh().
> >>2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
> >>3 aio_worker() will be executed in worker_thread().
> >>   Operation 2 and 3 are tracked by block layer's queue after Stefan's
> >>dropping io_flush() series.
> >>   Now if we stick to GMainContext to represent context concept,
> >>then when thread B want to aquire GMainContext used by thread A,
> >>all works setupped by A should be finished before B aquire it,
> >>otherwise B will execute some function supposed to work in A. The
> >>work refers to the three kind of aync functions above.
> >>   For this issue, we can solve it by different means:
> >>1 the event loop API doesn't guarentee work setupped by thread A
> >>will always be finished in A. This put a limitation to caller to
> >>consider thread switching. I talked on IRC with Stefan, and thinks
> >>it is possible for the nested user block layer, but I still want to
> >>avoid this to the flat user above block layer.
> >>2 ask caller to finish all pending operations.
> >>   2.1 glib GMainContext API plus custom API such as aio_wait(). This is
> >>bad that detail under GMainContext is exposed to caller. Since
> >>operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
> >>is called in thread setupped it, 1 need to be added in the track
> >>queue, or in the call chain of aio_wait(), check the queue plus check
> >>operation 1. Perhaps add a custom function ask caller to call as
> >>context_work_flush()?
> >   If a well named API do the flush work present, using Glib API plus
> >it seems also OK, and can avoid wrapper. I guess
> >bdrv_drain_all(GMainContext *ctx, ...) can do it.
> >
>   I haven't found a good answer in gstream, but want to show
> some idea from my understanding.
> 
> Following is a brief picture of the current event loop in qemu,
> Alex's timer for AioContext is also drawn here:
> 
>                                               ========================
>                                               || I/O thread in vl.c ||
>                                               ========================
>                                                           |
>                                                  run loop |
>                                                           |
> ====================                                      |
> ||    other       || qemu_set_fd_handler2()      =====================
> ||                ||-----------------------------||   Main_loop     ||
> ||(vnc, migration)||                   |         =====================
> ====================                   |          GLib    |
>                                        |    event loop API|
>                   qemu_set_fd_handler()|                  |
>                        -----------------          ====================
>                        |                          ||  GMainContext  ||
>                        |                          ====================
> ==========             |   (should it be removed?)        |
> ||  hw  ||--------------------------------------          |GSouce
> ==========                      |               |         |Attach
>                                 |  main_loop_tlg|         |
>                    qemu_bh_***()|               |         |
>                                 |               |         |
>                           ======|===============|=======================
>                           ||    |               |                     ||
> ===========               ||  ======   ==================   =======   ||
> || block ||---------------||  | BH |   | TimerListGroup |   | Fd  |   ||
> ===========  qemu_bh_***()||  ======   ==================   =======   ||
>            qemu_aio_wait()||                                          ||
>  qemu_aio_set_fd_handler()||  AioContext                              ||
>                           || (block layer's event collection)         ||
>                            =============================================
> 
> 
>   The main issue here is that components are tightly bind together and
> no clear layer represent the thread and event loop API. Block and hw
> code are inter acting with AioContext, so both GMainContext and
> AioContext are playing the role. I hope to form a library for block,
> So need to pick up one to provide event loop, the choice seems to be:
> 1 GMainContext.
> 2 AioContext.
> 3 Encapsulation, such as GlibQContext.
> 
>   1) and 2) would not be perfect since non standard glib event loop will
> be exposed, 3) will shows a unified interface similar to glib main loop,
> but more code adjust. After some thinking, I guess AioContext is the
> easiest way, which represent the block's own event loop, and give up
> using glib event loop at this level, just add custom API as
> block_iterate(). Briefly, bdrv_read will becomes:
> int bdrv_read(AioContext *ctx, ....);

I don't understand why you want to add AioContext *ctx to bdrv_read().
The synchronous APIs already work fine since no event loop integration
is necessary at all (the event loop is hidden inside the synchronous
function).

Since AioContext provides a GSource, integrating with an application's
glib event loop should also be easy.  The only hard part is timers,
since we use nanosecond timers - there we should just round up to
millisecond granularity to libqblock.  The nanosecond timers aren't
critical in libqblock, only for running guests.

Stefan

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

[Wenchao Xia]

于 2013-8-21 16:45, Stefan Hajnoczi 写道:
> On Tue, Aug 20, 2013 at 05:59:58PM +0800, Wenchao Xia wrote:
>> 于 2013-8-16 16:12, Wenchao Xia 写道:
>>> 于 2013-8-16 15:15, Wenchao Xia 写道:
>>>> 于 2013-8-16 0:32, Michael Roth 写道:
>>>>> Quoting Michael Roth (2013-08-15 10:23:20)
>>>>>> Quoting Wenchao Xia (2013-08-13 03:44:39)
>>>>>>> 于 2013-8-13 1:01, Michael Roth 写道:
>>>>>>>> Quoting Paolo Bonzini (2013-08-12 02:30:28)
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> Note that the nested event loops in QEMU are _very_ different from
>>>>>>>>> glib nested event loops.  In QEMU, nested event loops only run block
>>>>>>>>> layer events.  In glib, they run all events.  That's why you need
>>>>>>>>> AioContext.
>>>>>>>>
>>>>>>>> Would it be possible to use glib for our nested loops as well by just
>>>>>>>> setting a higher priority for the AioContext GSource?
>>>>>>>>
>>>>>>>> Stefan and I were considering how we could make use of his "drop
>>>>>>>> ioflush" patches to use a common mechanism to register fd events, but
>>>>>>>> still allow us to distinguish between AioContext and non-AioContext
>>>>>>>> for nested loops. I was originally thinking of using prepare()
>>>>>>>> functions
>>>>>>>> to filter out non-AioContext events, but that requires we implement
>>>>>>>> on GSource's with that in mind, and non make use of pre-baked ones
>>>>>>>> like GIOChannel's, and bakes block stuff into every event source
>>>>>>>> implementation.
>>>>>>>>
>>>>>>>     Besides priority, also g_source_set_can_recurse() can help.
>>>>>>>     With a deeper think, I found a harder problem:
>>>>>>> g_main_context_acquire() and g_main_context_release(). In release,
>>>>>>> pending BH/IO call back need to be cleared, but this action can't
>>>>>>> be triggered automatically when user call g_main_context_release().
>>>>>>
>>>>>> I don't understand why this is a requirement, gmctx_acquire/release
>>>>>> ensure
>>>>>> that only one thread attempts to iterate the main loop at a time. this
>>>>>> isn't currently an issue in qemu, and if we re-implemented
>>>>>> qemu_aio_wait()
>>>>>> to use the same glib interfaces, the tracking of in-flight requests
>>>>>> would
>>>>>> be moved to the block layer via Stefan's 'drop io_flush' patches, which
>>>>>> moves that block-specific logic out of the main loop/AioContext GSource
>>>>>> by design. Are there other areas where you see this as a problem?
>>>>>
>>>>> I think I understand better what you're referring to, you mean that
>>>>> if qemu_aio_wait was called, and was implementated to essentially call
>>>>> g_main_context_iterate(), that after, say, 1 iteration, the
>>>>> iothread/dataplane thread might acquire the main loop and dispatch
>>>>> block/non-block events between qemu_aio_wait() returned? The simple
>>>>> approach would be to have qemu_aio_wait() call
>>>>> g_main_context_acquire/release
>>>>> at the start end of the function, which would ensure that this never
>>>>> happens.
>>>>>
>>>>    qemu_aio_wait() is relative simple to resolve by
>>>> g_main_context_acquire(), but also shows additional code needed
>>>> for a thread switch:
>>>> (guess following is the plan to implement qemu_aio_wait())
>>>> qemu_aio_wait(GMainContext *ctx)
>>>> {
>>>>      return g_main_context(ctx, PRI_BLOCK);
>>>> }
>>>> at caller:
>>>> {
>>>>      while (qemu_aio_wait(ctx) {
>>>>          ;
>>>>      }
>>>>      g_main_context_release(ctx);
>>>> }
>>>>    The above code shows that, in *ctx switch operation, there is
>>>> more than glib's own event loop API envolved, qemu_aio_wait(). So
>>>> it referenced to a question: what data structure
>>>> should be used to represent context concept and control the thread
>>>> switching behavior?  It is better to have a clear layer, GMainContext or
>>>> GlibQContext, instead of GMainContext plus custom function. The caller
>>>> reference to at least two: nested user block layer, flat user above
>>>> block layer.
>>>>    In my opinion, this problem is brought by Gsource AioContext, Take
>>>> the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
>>>> an example, there are aync following operations involved for AioContext:
>>>> 1 the bdrv_cb() will be executed in bdrv_co_em_bh().
>>>> 2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
>>>> 3 aio_worker() will be executed in worker_thread().
>>>>    Operation 2 and 3 are tracked by block layer's queue after Stefan's
>>>> dropping io_flush() series.
>>>>    Now if we stick to GMainContext to represent context concept,
>>>> then when thread B want to aquire GMainContext used by thread A,
>>>> all works setupped by A should be finished before B aquire it,
>>>> otherwise B will execute some function supposed to work in A. The
>>>> work refers to the three kind of aync functions above.
>>>>    For this issue, we can solve it by different means:
>>>> 1 the event loop API doesn't guarentee work setupped by thread A
>>>> will always be finished in A. This put a limitation to caller to
>>>> consider thread switching. I talked on IRC with Stefan, and thinks
>>>> it is possible for the nested user block layer, but I still want to
>>>> avoid this to the flat user above block layer.
>>>> 2 ask caller to finish all pending operations.
>>>>    2.1 glib GMainContext API plus custom API such as aio_wait(). This is
>>>> bad that detail under GMainContext is exposed to caller. Since
>>>> operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
>>>> is called in thread setupped it, 1 need to be added in the track
>>>> queue, or in the call chain of aio_wait(), check the queue plus check
>>>> operation 1. Perhaps add a custom function ask caller to call as
>>>> context_work_flush()?
>>>    If a well named API do the flush work present, using Glib API plus
>>> it seems also OK, and can avoid wrapper. I guess
>>> bdrv_drain_all(GMainContext *ctx, ...) can do it.
>>>
>>    I haven't found a good answer in gstream, but want to show
>> some idea from my understanding.
>>
>> Following is a brief picture of the current event loop in qemu,
>> Alex's timer for AioContext is also drawn here:
>>
>>                                                ========================
>>                                                || I/O thread in vl.c ||
>>                                                ========================
>>                                                            |
>>                                                   run loop |
>>                                                            |
>> ====================                                      |
>> ||    other       || qemu_set_fd_handler2()      =====================
>> ||                ||-----------------------------||   Main_loop     ||
>> ||(vnc, migration)||                   |         =====================
>> ====================                   |          GLib    |
>>                                         |    event loop API|
>>                    qemu_set_fd_handler()|                  |
>>                         -----------------          ====================
>>                         |                          ||  GMainContext  ||
>>                         |                          ====================
>> ==========             |   (should it be removed?)        |
>> ||  hw  ||--------------------------------------          |GSouce
>> ==========                      |               |         |Attach
>>                                  |  main_loop_tlg|         |
>>                     qemu_bh_***()|               |         |
>>                                  |               |         |
>>                            ======|===============|=======================
>>                            ||    |               |                     ||
>> ===========               ||  ======   ==================   =======   ||
>> || block ||---------------||  | BH |   | TimerListGroup |   | Fd  |   ||
>> ===========  qemu_bh_***()||  ======   ==================   =======   ||
>>             qemu_aio_wait()||                                          ||
>>   qemu_aio_set_fd_handler()||  AioContext                              ||
>>                            || (block layer's event collection)         ||
>>                             =============================================
>>
>>
>>    The main issue here is that components are tightly bind together and
>> no clear layer represent the thread and event loop API. Block and hw
>> code are inter acting with AioContext, so both GMainContext and
>> AioContext are playing the role. I hope to form a library for block,
>> So need to pick up one to provide event loop, the choice seems to be:
>> 1 GMainContext.
>> 2 AioContext.
>> 3 Encapsulation, such as GlibQContext.
>>
>>    1) and 2) would not be perfect since non standard glib event loop will
>> be exposed, 3) will shows a unified interface similar to glib main loop,
>> but more code adjust. After some thinking, I guess AioContext is the
>> easiest way, which represent the block's own event loop, and give up
>> using glib event loop at this level, just add custom API as
>> block_iterate(). Briefly, bdrv_read will becomes:
>> int bdrv_read(AioContext *ctx, ....);
>
> I don't understand why you want to add AioContext *ctx to bdrv_read().
> The synchronous APIs already work fine since no event loop integration
> is necessary at all (the event loop is hidden inside the synchronous
> function).
>
   OK... I used a wrong example. It should be bdrv_aio_readv(). At this
level, do you think AioContext * should be used, instead of
GMainContext *?

> Since AioContext provides a GSource, integrating with an application's
> glib event loop should also be easy.  The only hard part is timers,
> since we use nanosecond timers - there we should just round up to
> millisecond granularity to libqblock.  The nanosecond timers aren't
> critical in libqblock, only for running guests.
>
> Stefan
>


-- 
Best Regards

Wenchao Xia

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

[Stefan Hajnoczi]

On Wed, Aug 21, 2013 at 05:33:41PM +0800, Wenchao Xia wrote:
> 于 2013-8-21 16:45, Stefan Hajnoczi 写道:
> >On Tue, Aug 20, 2013 at 05:59:58PM +0800, Wenchao Xia wrote:
> >>于 2013-8-16 16:12, Wenchao Xia 写道:
> >>>于 2013-8-16 15:15, Wenchao Xia 写道:
> >>>>于 2013-8-16 0:32, Michael Roth 写道:
> >>>>>Quoting Michael Roth (2013-08-15 10:23:20)
> >>>>>>Quoting Wenchao Xia (2013-08-13 03:44:39)
> >>>>>>>于 2013-8-13 1:01, Michael Roth 写道:
> >>>>>>>>Quoting Paolo Bonzini (2013-08-12 02:30:28)
> >>>>>>>>>>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.
> >>>>>>>>>
> >>>>>>>>>Note that the nested event loops in QEMU are _very_ different from
> >>>>>>>>>glib nested event loops.  In QEMU, nested event loops only run block
> >>>>>>>>>layer events.  In glib, they run all events.  That's why you need
> >>>>>>>>>AioContext.
> >>>>>>>>
> >>>>>>>>Would it be possible to use glib for our nested loops as well by just
> >>>>>>>>setting a higher priority for the AioContext GSource?
> >>>>>>>>
> >>>>>>>>Stefan and I were considering how we could make use of his "drop
> >>>>>>>>ioflush" patches to use a common mechanism to register fd events, but
> >>>>>>>>still allow us to distinguish between AioContext and non-AioContext
> >>>>>>>>for nested loops. I was originally thinking of using prepare()
> >>>>>>>>functions
> >>>>>>>>to filter out non-AioContext events, but that requires we implement
> >>>>>>>>on GSource's with that in mind, and non make use of pre-baked ones
> >>>>>>>>like GIOChannel's, and bakes block stuff into every event source
> >>>>>>>>implementation.
> >>>>>>>>
> >>>>>>>    Besides priority, also g_source_set_can_recurse() can help.
> >>>>>>>    With a deeper think, I found a harder problem:
> >>>>>>>g_main_context_acquire() and g_main_context_release(). In release,
> >>>>>>>pending BH/IO call back need to be cleared, but this action can't
> >>>>>>>be triggered automatically when user call g_main_context_release().
> >>>>>>
> >>>>>>I don't understand why this is a requirement, gmctx_acquire/release
> >>>>>>ensure
> >>>>>>that only one thread attempts to iterate the main loop at a time. this
> >>>>>>isn't currently an issue in qemu, and if we re-implemented
> >>>>>>qemu_aio_wait()
> >>>>>>to use the same glib interfaces, the tracking of in-flight requests
> >>>>>>would
> >>>>>>be moved to the block layer via Stefan's 'drop io_flush' patches, which
> >>>>>>moves that block-specific logic out of the main loop/AioContext GSource
> >>>>>>by design. Are there other areas where you see this as a problem?
> >>>>>
> >>>>>I think I understand better what you're referring to, you mean that
> >>>>>if qemu_aio_wait was called, and was implementated to essentially call
> >>>>>g_main_context_iterate(), that after, say, 1 iteration, the
> >>>>>iothread/dataplane thread might acquire the main loop and dispatch
> >>>>>block/non-block events between qemu_aio_wait() returned? The simple
> >>>>>approach would be to have qemu_aio_wait() call
> >>>>>g_main_context_acquire/release
> >>>>>at the start end of the function, which would ensure that this never
> >>>>>happens.
> >>>>>
> >>>>   qemu_aio_wait() is relative simple to resolve by
> >>>>g_main_context_acquire(), but also shows additional code needed
> >>>>for a thread switch:
> >>>>(guess following is the plan to implement qemu_aio_wait())
> >>>>qemu_aio_wait(GMainContext *ctx)
> >>>>{
> >>>>     return g_main_context(ctx, PRI_BLOCK);
> >>>>}
> >>>>at caller:
> >>>>{
> >>>>     while (qemu_aio_wait(ctx) {
> >>>>         ;
> >>>>     }
> >>>>     g_main_context_release(ctx);
> >>>>}
> >>>>   The above code shows that, in *ctx switch operation, there is
> >>>>more than glib's own event loop API envolved, qemu_aio_wait(). So
> >>>>it referenced to a question: what data structure
> >>>>should be used to represent context concept and control the thread
> >>>>switching behavior?  It is better to have a clear layer, GMainContext or
> >>>>GlibQContext, instead of GMainContext plus custom function. The caller
> >>>>reference to at least two: nested user block layer, flat user above
> >>>>block layer.
> >>>>   In my opinion, this problem is brought by Gsource AioContext, Take
> >>>>the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
> >>>>an example, there are aync following operations involved for AioContext:
> >>>>1 the bdrv_cb() will be executed in bdrv_co_em_bh().
> >>>>2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
> >>>>3 aio_worker() will be executed in worker_thread().
> >>>>   Operation 2 and 3 are tracked by block layer's queue after Stefan's
> >>>>dropping io_flush() series.
> >>>>   Now if we stick to GMainContext to represent context concept,
> >>>>then when thread B want to aquire GMainContext used by thread A,
> >>>>all works setupped by A should be finished before B aquire it,
> >>>>otherwise B will execute some function supposed to work in A. The
> >>>>work refers to the three kind of aync functions above.
> >>>>   For this issue, we can solve it by different means:
> >>>>1 the event loop API doesn't guarentee work setupped by thread A
> >>>>will always be finished in A. This put a limitation to caller to
> >>>>consider thread switching. I talked on IRC with Stefan, and thinks
> >>>>it is possible for the nested user block layer, but I still want to
> >>>>avoid this to the flat user above block layer.
> >>>>2 ask caller to finish all pending operations.
> >>>>   2.1 glib GMainContext API plus custom API such as aio_wait(). This is
> >>>>bad that detail under GMainContext is exposed to caller. Since
> >>>>operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
> >>>>is called in thread setupped it, 1 need to be added in the track
> >>>>queue, or in the call chain of aio_wait(), check the queue plus check
> >>>>operation 1. Perhaps add a custom function ask caller to call as
> >>>>context_work_flush()?
> >>>   If a well named API do the flush work present, using Glib API plus
> >>>it seems also OK, and can avoid wrapper. I guess
> >>>bdrv_drain_all(GMainContext *ctx, ...) can do it.
> >>>
> >>   I haven't found a good answer in gstream, but want to show
> >>some idea from my understanding.
> >>
> >>Following is a brief picture of the current event loop in qemu,
> >>Alex's timer for AioContext is also drawn here:
> >>
> >>                                               ========================
> >>                                               || I/O thread in vl.c ||
> >>                                               ========================
> >>                                                           |
> >>                                                  run loop |
> >>                                                           |
> >>====================                                      |
> >>||    other       || qemu_set_fd_handler2()      =====================
> >>||                ||-----------------------------||   Main_loop     ||
> >>||(vnc, migration)||                   |         =====================
> >>====================                   |          GLib    |
> >>                                        |    event loop API|
> >>                   qemu_set_fd_handler()|                  |
> >>                        -----------------          ====================
> >>                        |                          ||  GMainContext  ||
> >>                        |                          ====================
> >>==========             |   (should it be removed?)        |
> >>||  hw  ||--------------------------------------          |GSouce
> >>==========                      |               |         |Attach
> >>                                 |  main_loop_tlg|         |
> >>                    qemu_bh_***()|               |         |
> >>                                 |               |         |
> >>                           ======|===============|=======================
> >>                           ||    |               |                     ||
> >>===========               ||  ======   ==================   =======   ||
> >>|| block ||---------------||  | BH |   | TimerListGroup |   | Fd  |   ||
> >>===========  qemu_bh_***()||  ======   ==================   =======   ||
> >>            qemu_aio_wait()||                                          ||
> >>  qemu_aio_set_fd_handler()||  AioContext                              ||
> >>                           || (block layer's event collection)         ||
> >>                            =============================================
> >>
> >>
> >>   The main issue here is that components are tightly bind together and
> >>no clear layer represent the thread and event loop API. Block and hw
> >>code are inter acting with AioContext, so both GMainContext and
> >>AioContext are playing the role. I hope to form a library for block,
> >>So need to pick up one to provide event loop, the choice seems to be:
> >>1 GMainContext.
> >>2 AioContext.
> >>3 Encapsulation, such as GlibQContext.
> >>
> >>   1) and 2) would not be perfect since non standard glib event loop will
> >>be exposed, 3) will shows a unified interface similar to glib main loop,
> >>but more code adjust. After some thinking, I guess AioContext is the
> >>easiest way, which represent the block's own event loop, and give up
> >>using glib event loop at this level, just add custom API as
> >>block_iterate(). Briefly, bdrv_read will becomes:
> >>int bdrv_read(AioContext *ctx, ....);
> >
> >I don't understand why you want to add AioContext *ctx to bdrv_read().
> >The synchronous APIs already work fine since no event loop integration
> >is necessary at all (the event loop is hidden inside the synchronous
> >function).
> >
>   OK... I used a wrong example. It should be bdrv_aio_readv(). At this
> level, do you think AioContext * should be used, instead of
> GMainContext *?

Maybe but...

> >Since AioContext provides a GSource, integrating with an application's
> >glib event loop should also be easy.  The only hard part is timers,
> >since we use nanosecond timers - there we should just round up to
> >millisecond granularity to libqblock.  The nanosecond timers aren't
> >critical in libqblock, only for running guests.

...we already have a GSource that applications can integrate into their
event loop.  Why do you want to modify the code instead of just exposing
the GSource?

Stefan

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

[Alex Bligh]

--On 21 August 2013 10:45:53 +0200 Stefan Hajnoczi <stefanha@gmail.com> 
wrote:

> Since AioContext provides a GSource, integrating with an application's
> glib event loop should also be easy.  The only hard part is timers,
> since we use nanosecond timers - there we should just round up to
> millisecond granularity to libqblock.

... and there is qemu_timeout_ns_to_ms for this purpose which does
the rounding right.

-- 
Alex Bligh

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

[Alex Bligh]

--On 10 August 2013 11:24:01 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> 
wrote:

>   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:

This may well clash with the aio / timers patch series. I think the
current direction is to introduce a dummy AioContext in place
of glib's main loop. That's the bit I have not done yet in the (just
posted) v9 of this patch (and your approach should work with the
v8 approach).

If we went this way we'd add a TimerListGroup to the the GSource
sub class I guess. This does seem a little less hacky. However
we'd have to make g_source timeouts work in nanoseconds rather
than milliseconds.

-- 
Alex Bligh