apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@rkbloom.net
Subject Re: RESEND: PATCH: Update requested events for a descriptor that is already in a poll set
Date Mon, 15 Mar 2004 18:11:09 GMT
A couple of comments:

1)  This patch isn't complete.  At the very least we need a patch to the header
that includes doxygen comments that document this new API.  Preferably, a new
patch would also include a patch to testpoll to test this feature.  It should
also include a stub function in all non-unix poll files, so that other builds
don't fail.

Quoting Bill Stoddard <bill@wstoddard.com>:

> Robert Norris wrote:

> > Attached is a patch that adds a function apr_pollset_update(). This
> > updates the list of requested events for the descriptor in the
> > underlying implementation for a descriptor that is already in a pollset.
> > 
> > It works well enough, but I'm not happy with it because it requires a
> > search through the pollset (just as apr_pollset_remove() does). On a
> > server with hundreds or thousands of descriptors in the set and changing
> > events often, this might end up being too expensive.
> > 
> > The optimisation that I'd like to do is to maintain a pointer (by index
> > or explicit) for each descriptor that points to the descriptors'
> > underlying pollfd structure, and just use this. This would lose the
> > ability to enter a socket into the set multiple times with different
> > events, as is currently possible, but would eliminate the search.

I'm not sure that ability really exists currently (instead of it just being an
accident of implementation), and if it does I am positive that it isn't
portable.  Consider the simple case where the underlying system is using select
instead() of poll().  When using select, you can't have the same descriptor in
the pollset twice, the system doesn't support it.

> > The only problem with this is that such a pointer would really belong in
> > apr_pollfd_t, except that its specific to the poll() implementation, and
> > that type is not opaque. So I'd appreciate some pointers on how to best
> > do this.

The easiest way is to create a union of incomplete types that actually implement
the pointer.  This is how pollfd_t has a pointer to the apr_file_t and
apr_socket_t structures.  That will make the code portable at least, and users
outside of APR won't be able to mess up the pointer.


> > ------------------------------------------------------------------------
> > 
> > diff -U3 -r /home/box/downloads/apr-0.9.4/include/apr_poll.h
> apr-0.9.4/include/apr_poll.h
> > --- /home/box/downloads/apr-0.9.4/include/apr_poll.h	2003-03-06
> 08:22:26.000000000 +1100
> > +++ apr-0.9.4/include/apr_poll.h	2004-03-05 08:57:56.000000000 +1100
> > @@ -270,6 +270,14 @@
> >                                               const apr_pollfd_t
> *descriptor);
> >  
> >  /**
> > + * Update the requested events for the descriptor in the underlying
> implementation
> > + * @param pollset The pollset that the descriptor is a member of
> > + * @param descriptor The descriptor to update
> > + */
> > +APR_DECLARE(apr_status_t) apr_pollset_update(apr_pollset_t *pollset,
> > +                                             const apr_pollfd_t
> *descriptor);
> > +
> > +/**
> >   * Block for activity on the descriptor(s) in a pollset
> >   * @param pollset The pollset to use
> >   * @param timeout Timeout in microseconds
> > diff -U3 -r /home/box/downloads/apr-0.9.4/poll/unix/poll.c
> apr-0.9.4/poll/unix/poll.c
> > --- /home/box/downloads/apr-0.9.4/poll/unix/poll.c	2003-01-07
> 11:52:56.000000000 +1100
> > +++ apr-0.9.4/poll/unix/poll.c	2004-03-05 09:26:54.000000000 +1100
> > @@ -515,6 +515,55 @@
> >      return APR_NOTFOUND;
> >  }
> >  
> > +APR_DECLARE(apr_status_t) apr_pollset_update(apr_pollset_t *pollset,
> > +                                             const apr_pollfd_t
> *descriptor)
> > +{
> > +    apr_uint32_t i;
> > +
> > +#ifndef HAVE_POLL
> > +    apr_os_sock_t fd;
> > +
> > +    if (descriptor->desc_type == APR_POLL_SOCKET) {
> > +        fd = descriptor->desc.s->socketdes;
> > +    }
> > +    else {
> > +#if !APR_FILES_AS_SOCKETS
> > +        return APR_EBADF;
> > +#else
> > +        fd = descriptor->desc.f->filedes;
> > +#endif
> > +    }
> > +#endif /* !HAVE_POLL */
> > +
> > +    for (i = 0; i < pollset->nelts; i++) {
> > +        if (descriptor->desc.s == pollset->query_set[i].desc.s) {
> > +            /* Found an instance of the fd: update its events */
> > +            pollset->query_set[i].reqevents = descriptor->reqevents;
> > +
> > +#ifdef HAVE_POLL
> > +            pollset->pollset[i].events =
> get_event(descriptor->reqevents);
> > +#else
> > +            if (descriptor->reqevents & APR_POLLIN) {
> > +                FD_SET(fd, &(pollset->readset));
> > +            }
> > +            if (descriptor->reqevents & APR_POLLOUT) {
> > +                FD_SET(fd, &(pollset->writeset));
> > +            }
> > +            if (descriptor->reqevents &
> > +                (APR_POLLPRI | APR_POLLERR | APR_POLLHUP | APR_POLLNVAL))
> {
> > +                FD_SET(fd, &(pollset->exceptset));
> > +            }
> > +#endif

Before you can do the FD_SET calls, you must do FD_CLR.  Otherwise, you won't
remove the old reqevents from the fd_set.

> > +        }
> > +    }
> > +    
> > +    if (i == pollset->nelts) {
> > +        return APR_NOTFOUND;
> > +    }
> > +
> > +    return APR_SUCCESS;
> > +}
> > +
> >  #ifdef HAVE_POLL
> >  APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pollset_t *pollset,
> >                                             apr_interval_time_t timeout,

In general, I like the idea for this API, but I would much rather that it was
implemented as:

apr_status_t apr_pollset_update(....) {
    apr_pollset_remove(...);
    apr_pollset_add(...);
}

This would remove the possible bugs from forgetting to call things like FD_CLR.
 It would also mean that adding this function to all poll implementations would
be trivial.  With the index pointer added, it also wouldn't be much less
performant than the original version.

Ryan

Mime
View raw message