[ovs-dev] [PATCH] poll-loop: windows poll_block implementation

[Ben Pfaff]

On Wed, Dec 18, 2013 at 06:26:31PM -0800, Linda Sun wrote:
> Use WaitForMultipleObjects for polling on windows.  This works on all kinds
>  of objects, e.g. sockets, files, especially ioctl calls to the kernel.
>  poll_fd_wait_event() is used if events need to be passed to pollfds.
> latch is signaled with event, to be waited/polled by WaitForMultipleObjects()
>  as well.
> Changed array of fds to hmap to check for duplicate fds.
> 
> Signed-off-by: Linda Sun <lsun at vmware.com>

Thanks!

It appears to me that latch-windows doesn't use fds[] at all.  If so,
then it would make sense to omit them from the data structure.

I don't think that it's safe to do the operations that latch_poll()
and latch_set() do without synchronization.  I suggest putting a lock
around those two whole functions.  (Probably, one global lock is
plenty.)

> +struct poll_node {
> +    struct hmap_node    hmap_node;
> +    struct pollfd       poll_fd;    /* Events to pass to time_poll() */
> +    HANDLE              wevent;     /* events for waitformultipleobjects */
> +    const char          *where;     /* where each pollfd was created */

We don't generally line up member names like this.  Just use a space
between the type and the member name.

> +/* Look up the node with same fd and wevent */
> +static struct poll_node *
> +poll_fd_node_find(struct poll_loop *loop, int fd, uint32_t wevent)
> +{
> +    struct poll_node *node;
> +
> +    HMAP_FOR_EACH_WITH_HASH(node, hmap_node, hash_2words(fd, wevent), &loop->poll_nodes) {

Please put a space after HASH_FUNC.  Also, the line is too line,
please limit lines to 79 characters.

> +        if (node->poll_fd.fd == fd && node->wevent == wevent) {
> +            return node;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  /* Registers 'fd' as waiting for the specified 'events' (which should be POLLIN
>   * or POLLOUT or POLLIN | POLLOUT).  The following call to poll_block() will
>   * wake up when 'fd' becomes ready for one or more of the requested events.
> @@ -63,23 +83,37 @@ static struct poll_loop *poll_loop(void);
>   * automatically provide the caller's source file and line number for
>   * 'where'.) */
>  void
> -poll_fd_wait_at(int fd, short int events, const char *where)
> +poll_fd_wait_at(int fd, HANDLE wevent, short int events, const char *where)
>  {

This function really needs a comment describing how either fd or
wevent is used, depending on OS.

>      struct poll_loop *loop = poll_loop();
> +    struct poll_node *node;
>  
>      COVERAGE_INC(poll_fd_wait);
> -    if (loop->n_waiters >= loop->allocated_waiters) {
> -        loop->where = x2nrealloc(loop->where, &loop->allocated_waiters,
> -                                 sizeof *loop->where);
> -        loop->pollfds = xrealloc(loop->pollfds,
> -                                 (loop->allocated_waiters
> -                                  * sizeof *loop->pollfds));
> +
> +#ifdef _WIN32
> +    /* null event cannot be polled */
> +    if (wevent == 0) {
> +        VLOG_ERR("No event to wait fd %d\n", fd);

The \n is unnecessary.

> +        return;
>      }
> +#endif
>  
> -    loop->where[loop->n_waiters] = where;
> -    loop->pollfds[loop->n_waiters].fd = fd;
> -    loop->pollfds[loop->n_waiters].events = events;
> -    loop->n_waiters++;
> +    /* check for duplicate.  If found, "or" the event */
> +    node = poll_fd_node_find(loop, fd, wevent);
> +    if (node) {
> +        node->poll_fd.events |= events;
> +    } else {
> +        node = xzalloc(sizeof *node);
> +        if (node == NULL) {

This NULL check is unnecessary because xzalloc never returns NULL.

> +            return;
> +        }
> +        node->where = where;
> +        node->poll_fd.fd = fd;
> +        node->wevent = wevent;
> +        node->poll_fd.events = events;
> +        hmap_insert(&loop->poll_nodes, &node->hmap_node,
> +            hash_2words(fd, wevent));
> +    }
>  }
>  
>  /* Causes the following call to poll_block() to block for no more than 'msec'

> @@ -227,7 +265,29 @@ poll_block(void)
>      }
>  
>      timewarp_wait();
> -    retval = time_poll(loop->pollfds, loop->n_waiters,
> +    pollfds = xzalloc(hmap_count(&loop->poll_nodes) * sizeof *pollfds);

Please use xmalloc() here, it is unnecessary to zero the whole block.

> +    if (pollfds == NULL) {

This NULL check is unnecessary because xzalloc never returns NULL.

> +        return;
> +    }
> +
> +#ifdef _WIN32
> +    wevents = xzalloc(hmap_count(&loop->poll_nodes) * sizeof *wevents);
> +    if (wevents == NULL) {

This NULL check is unnecessary because xzalloc never returns NULL.

> +        free(pollfds);
> +        return;
> +    }
> +#endif
> +
> +    /* populate with all the fds and events */
> +    HMAP_FOR_EACH(node, hmap_node, &loop->poll_nodes) {
> +        memcpy(&pollfds[i], &node->poll_fd, sizeof node->poll_fd);
> +#ifdef _WIN32
> +        wevents[i] = node->wevent;
> +#endif
> +        i++;
> +    }
> +
> +    retval = time_poll(pollfds, hmap_count(&loop->poll_nodes), wevents,
>                         loop->timeout_when, &elapsed);
>      if (retval < 0) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> @@ -235,18 +295,23 @@ poll_block(void)
>      } else if (!retval) {
>          log_wakeup(loop->timeout_where, NULL, elapsed);
>      } else if (get_cpu_usage() > 50 || VLOG_IS_DBG_ENABLED()) {
> -        size_t i;
> -
> -        for (i = 0; i < loop->n_waiters; i++) {
> -            if (loop->pollfds[i].revents) {
> -                log_wakeup(loop->where[i], &loop->pollfds[i], 0);
> +        HMAP_FOR_EACH(node, hmap_node, &loop->poll_nodes) {
> +            if (node->poll_fd.revents) {

I don't think this is going to work because that pollfd didn't get
updated by time_poll().

> +                log_wakeup(node->where, &node->poll_fd, 0);
>              }
>          }
>      }
>  
> +    HMAP_FOR_EACH_SAFE(node, next, hmap_node, &loop->poll_nodes) {
> +        hmap_remove(&loop->poll_nodes, &node->hmap_node);
> +        free(node);
> +    }
> +
>      loop->timeout_when = LLONG_MAX;
>      loop->timeout_where = NULL;
> -    loop->n_waiters = 0;
> +    free(pollfds);
> +    if (wevents)
> +        free(wevents);

Needs {} around the conditional statement.

>      /* Handle any pending signals before doing anything else. */
>      fatal_signal_run();
> @@ -258,9 +323,14 @@ static void
>  free_poll_loop(void *loop_)
>  {
>      struct poll_loop *loop = loop_;
> +    struct poll_node *node, *next;
> +
> +    HMAP_FOR_EACH_SAFE(node, next, hmap_node, &loop->poll_nodes) {

Please add a space after HMAP_FOR_EACH_SAFE.

> +        hmap_remove(&loop->poll_nodes, &node->hmap_node);
> +        free(node);
> +    }
>  
> -    free(loop->pollfds);
> -    free(loop->where);
> +    hmap_destroy(&loop->poll_nodes);
>      free(loop);
>  }
>  

> +        if (retval < 0) {
> +            /* This will be replace by a win error to errno conversion function */

I'd add XXX to the comment to make it easier to spot.

> +            retval = -WSAGetLastError();
> +            retval = -EINVAL;
> +        }
> +#endif

Thanks,

Ben.