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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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