Re: [Xen-devel] [PATCH] Switch to poll() in cxenstored's IO loop

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-01-21 at 19:23 +0000, Wei Liu wrote:

> -             if (reopen_log_pipe[0] != -1 && FD_ISSET(reopen_log_pipe[0], 
> &inset)) {
> +             if (reopen_log_pipe[0] != -1 && reopen_log_pipe0_pollfd &&

Is it the case that reopen_log_pip0_pollfd != NULL iff
reopen_log_pipe[0] != -1 ? Would simplify things a bit?

> +                 !(reopen_log_pipe0_pollfd->revents & 
> ~(POLLIN|POLLOUT|POLLPRI)) &&

This represents an error case I think? Is there anything we can do about
it? If this sort of error occurs and we do nothing will we end up just
spinning because every subsequent poll will just return straight away?


> +                 (reopen_log_pipe0_pollfd->revents & POLLIN)) {

You only handle POLLIN, not POLLOUT or POLLPRI -- so should they not be
part of the error case above? I don't think you ask for them other than
on the conn->poll_fd?

(those three comments apply to the other instances of this pattern too)
[...]
> @@ -1939,21 +1981,27 @@ int main(int argc, char *argv[])
>                               if (talloc_free(conn) == 0)
>                                       continue;
>                       } else {
> -                             if (FD_ISSET(conn->fd, &inset))
> +                             if (conn->pollfd &&
> +                                 !(conn->pollfd->revents & 
> ~(POLLIN|POLLOUT|POLLPRI)) &&
> +                                 (conn->pollfd->revents & POLLIN))
>                                       handle_input(conn);
>                               if (talloc_free(conn) == 0)
>                                       continue;

Do you know what this construct is all about? Some sort of reference
count on conn? Anyway, I wonder if this means you will frequently miss
setting pollfd = NULL below?

talloc_free can apparently only return non-zero if a destructor fails,
which suggests that more often than not you will not make it as far as
checking POLLOUT.

This is all pre-existing though, so maybe it just works and there is
some magic I don't understand ;-)

>                               talloc_increase_ref_count(conn);
> -                             if (FD_ISSET(conn->fd, &outset))
> +                             if (conn->pollfd &&
> +                                 !(conn->pollfd->revents & 
> ~(POLLIN|POLLOUT|POLLPRI)) &&
> +                                 (conn->pollfd->revents & POLLOUT))
>                                       handle_output(conn);
>                               if (talloc_free(conn) == 0)
>                                       continue;
> +
> +                             conn->pollfd = NULL;
>                       }
>               }
>  
> -             max = initialize_set(&inset, &outset, *sock, *ro_sock,
> -                                  &timeout);
> +             initialize_fds(*sock, &sock_pollfd, *ro_sock, &ro_sock_pollfd,
> +                            &timeout);
>       }
>  }
>  
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index 492ca0d..f330a87 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -60,6 +60,8 @@ struct connection
>  
>       /* The file descriptor we came in on. */
>       int fd;
> +     /* The pollfd corresponding to fd. */
> +     struct pollfd *pollfd;
>  
>       /* Who am I? 0 for socket connections. */
>       unsigned int id;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel