[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 02/10] main-loop: switch to g_poll() on POSIX
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v3 02/10] main-loop: switch to g_poll() on POSIX hosts |
Date: |
Mon, 4 Feb 2013 16:07:20 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Feb 04, 2013 at 03:37:39PM +0100, Laszlo Ersek wrote:
> I assume this is complex because the pre-patch situation is overly
> complex as well:
>
> slirp -> fd_set
> iohandler -> fd_set
> unix windows
> ---------------------------------
> --------------------------------------------
> before after before after
> -------------- ----------------- ---------------------
> ---------------------
> glib -> fd_set glib -> fd_set glib -> GPollFD glib -> GPollFD
> fd_set -> GPollFD WaitObj -> GPollFD WaitObj -> GPollFD
>
> SELECT G_POLL G_POLL (glib/WaitObj) G_POLL
> (glib/WaitObj)
>
> fd_set -> GPollFD
> GPollFD -> fd_set
>
> SELECT (slirp/ioh.) SELECT (slirp/ioh.)
>
> (I'm ignoring the after-select / after-poll operations; they (should)
> mirror the pre-select / pre-poll ones.)
>
> No idea why the windows version has been mixing g_poll() and select().
> I'd hope that this series kills select() for uniformity's sake, but the
> 00/10 subject and the commit msg for this patch indicate otherwise.
This is why I CCed Fabien. I left the g_poll-followed-by-select
behavior. It may be to do with Windows treating sockets different from
other objects. Paolo may know the answer, too.
> "main-loop.c" is full of global variables which makes it a *pain* to read.
Yes.
> > + if (FD_ISSET(fd, &wfds)) {
> > + events |= G_IO_OUT | G_IO_ERR;
> > + }
> > + if (FD_ISSET(fd, &xfds)) {
> > + events |= G_IO_PRI;
> > + }
> > + if (events) {
> > + GPollFD pfd = {
> > + .fd = fd,
> > + .events = events,
> > + };
> > + g_array_append_val(gpollfds, pfd);
> > + }
> > + }
> > +}
>
> (I don't like "gpollfds" being global, but) this seems OK. It matches
> the glib docs and also How the Mapping Should Be Done (TM).
>
> This function corresponds to the "unix / after / fd_set -> GPollFD"
> entry of the diagram, and as such it is composed with "glib -> fd_set"
> (glib_select_fill()). I'll note that glib_select_fill() sets "xfds" for
> G_IO_ERR but not G_IO_PRI. (According to SUSv4, in our case xfds is the
> logical-or of "error pending" and "OOB/urgent pending".)
>
> We can assume that each entry stored by g_main_context_query() will have
> at least one of IN and OUT set in "events". Further assuming that glib
> follows its own documentation, that implies that G_IO_ERR will be set
> unconditionally (because it always accompanies each of IN and OUT).
> glib_select_fill() will map that to xfds, which we then map here to
> G_IO_PRI. The total effect is that G_IO_PRI is set on all file
> descriptors, always, even if we only try to write.
>
> I think ultimately support for OOB data should be killed throughout, as
> a policy, including xfds & G_IO_PRI. Pending errors are returned by
> read()/write()/etc just fine; see eg. libvirt commit d19149dd.
slirp uses OOB because it implements TCP. I don't think we can drop it.
However, later in the series we drop the glib_select_fill()
rfds/wfds/xfds conversion and use GPollFD directly. That solves these
conversion issues.
> > +
> > + gpollfds_to_select(ret);
> >
> > if (timeout > 0) {
> > qemu_mutex_lock_iothread();
> > @@ -327,6 +386,55 @@ void qemu_fd_register(int fd)
> > FD_CONNECT | FD_WRITE | FD_OOB);
> > }
> >
> > +static int pollfds_fill(GArray *pollfds, fd_set *rfds, fd_set *wfds,
> > + fd_set *xfds)
> > +{
> > + int nfds = -1;
>
> "nfds" shadows the global "nfds". The return value of pollfds_fill() is
> then assigned to the global "nfds". Not very elegant (but it's all
> rooted in the existense of the global nfds).
Global nfds is dropped later in the series. I had real trouble picking
good names due to the globals and ended up with temporary shadowing,
which is fixed later.
> > + if (FD_ISSET(fd, wfds)) {
> > + revents |= G_IO_OUT | G_IO_ERR;
> > + }
> > + if (FD_ISSET(fd, xfds)) {
> > + revents |= G_IO_PRI;
> > + }
> > + pfd->revents |= revents & pfd->events;
>
> I find this |= vs. = more confusing than the other two. "pfd->revents"
> is zero here.
Yes, it's an interesting feature of pollfds_poll(). It does not clobber
revents. This means you can apply additional *_poll()-style functions
before calling pollfds_poll(). I thought this was neat but I guess we
can drop it.
>
> > + }
> > +}
> > +
> > static int os_host_main_loop_wait(uint32_t timeout)
> > {
> > GMainContext *context = g_main_context_default();
> > @@ -382,12 +490,22 @@ static int os_host_main_loop_wait(uint32_t timeout)
> > * improve socket latency.
> > */
> >
> > + /* This back-and-forth between GPollFDs and select(2) is temporary.
> > We'll
> > + * drop it in a couple of patches, I promise :).
> > + */
> > + gpollfds_from_select();
> > + FD_ZERO(&rfds);
> > + FD_ZERO(&wfds);
> > + FD_ZERO(&xfds);
> > + nfds = pollfds_fill(gpollfds, &rfds, &wfds, &xfds);
> > if (nfds >= 0) {
> > select_ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv0);
> > if (select_ret != 0) {
> > timeout = 0;
> > + pollfds_poll(gpollfds, nfds, &rfds, &wfds, &xfds);
>
> This function shouldn't be invoked for "select_ret == -1" (in case
> that's possible here); "revents" will end up a copy of "events":
>
> "On failure, the objects pointed to by the readfds, writefds, and
> errorfds arguments shall not be modified."
Thanks, will fix.
>
> > }
> > }
> > + gpollfds_to_select(select_ret || g_poll_ret);
>
> I can see this logical-or mimics the return value below; however I think
> it's not robust. If one of the operands is -1, and the other is
> non-positive, then gpollfds_to_select() should not traverse the
> GPollFDs, but it will.
>
> ... Actually, "g_poll_ret" should have absolutely no effect on
> gpollfds_to_select(): "g_poll_ret" corresponds to the "poll_fds" array,
> which covers the glib-internal file descriptors and the WaitObjects.
> Here (= in the windows case), "gpollfds" covers only slirp & iohandler,
> thus only "select_ret" should be passed in.
Will take a look at this for the next version of this patch.
> I can't decide (yet) if any of the above is important; probably not.
> From a quick look at the tree at the series' end, most of it seems to be
> gone by then. I'll let you decide.
Thanks for the review. If I need to respin I'll address comments.
Stefan
[Qemu-devel] [PATCH v3 06/10] iohandler: switch to GPollFD, Stefan Hajnoczi, 2013/02/04
[Qemu-devel] [PATCH v3 05/10] slirp: switch to GPollFD, Stefan Hajnoczi, 2013/02/04
[Qemu-devel] [PATCH v3 04/10] slirp: slirp/slirp.c coding style cleanup, Stefan Hajnoczi, 2013/02/04