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 <l...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
