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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, 2013-01-22 at 14:25 +0000, Ian Campbell wrote:
> 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.
> 

Hah, now I get your point. ;-)

> > > > +                   !(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.
> 

We can reinitialize the pipe if error occurs, because this pipe is only
used within xenstored itself to notify log reload.

> > > > @@ -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?
> 

I presume there is some magic here? The original code is constructed
like this, I only did the necessary switch to use poll().


Wei.




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