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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-01-22 at 14:13 +0000, Wei Liu wrote:
> On Tue, 2013-01-22 at 09:47 +0000, Ian Campbell wrote:
> > 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?
> > 
> 
> Not really. Pollfd is valid iff set_fd() returns a valid pointer.

But set_fd takes reopen_log_pipe[0], doesn't it? So a necessary
precondition is that reopen_log_pipe[0] is != -1.

IOW whenever reopen_log_pip0_pollfd != NULL it is also true that
reopen_log_pipe[0] != -1 and so "if (reopen_log_pipe[0] != -1 &&
reopen_log_pipe0_pollfd)" is equivalent to just "if
(reopen_log_pipe0_pollfd)"

> set_fd() invokes realloc() which can fail.

In the case where reopen_log_pip0_pollfd == NULL it doesn't matter what
reopen_log_pipe[0] is though.

> > > +             !(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?
> > 
> > 
> 
> Yes, this represents an error. This indicates the pipe used to trigger
> reopen_log is broken. What's the motivation of reopening log file?

Rotation I should imagine.

>  I
> have no idea about the use case. But simply ignoring this error instead
> of crashing the process is better choice IMHO.

Agreed, but my concern was that the daemon would spin consuming 100%
CPU, which is nearly as bad as crashing.

> The whole pollfd set is reinitialized for every loop. So it is also
> possible that for the next loop it will success even previous poll
> fails?

I suppose it depends on the reason it failed. I would expect most of
them would be pretty final rather than temporary but I haven't looked at
the pipe docs.

> > > @@ -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 ;-)
> > 
> 
> The document of talloc_free() says that if the pointer is freed 0 is
> returned otherwise -1 is returned.
> 
> So if conn is successfully freed, than there is no need to reset pollfd
> in conn to NULL.

Doh, yes.

What about missing any pending POLLOUT in that case though, due to the
continue?

Ian.


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