Re: [Qemu-devel] [PATCH v3 02/10] main-loop: switch to g_poll() on POSIX hosts

[Stefan Hajnoczi]

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