apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Querna <c...@force-elite.com>
Subject Re: Pollset: Thread Safe & More
Date Thu, 23 Sep 2004 13:40:58 GMT
On Thu, 2004-09-23 at 11:38 +0100, Joe Orton wrote:
> On Wed, Sep 22, 2004 at 08:57:24PM -0600, Paul Querna wrote:
> > Attached is a Patch for apr_pollset_*. 
> > Changes Include:
> > - Replace HAS_* with USE_* to remove some complex #ifdef stuff.
> > - Partially thread safe (*)
> > - Removes the limitation on the number of sockets that you can add to a
> > pollset (**)
> 
> This is kind of neat.  But :)
> 
> w.r.t thread-safety: Pushing this stuff down into the APR pollset code
> means that all non-threaded users of the pollset API incur extra
> overhead for functionality which they don't need or care about.
> 
> In the event MPM, could you not, for instance, just keep two queues of
> "pollfds to remove" and "pollfds to add" which are handled with
> appropriate locking, and then process these queues calling _add and
> _remove appropriate after _poll() returns?  i.e. ensure that a single
> thread owns the pollset structure.

That is sort of the previous design.  The problem is, we don't know how
long a thread will be stuck inside the _poll(). It could be a tiny
amount of time in a busy server, but on a server w/ only one client
connection, it might take 10 seconds or some such to leave a _poll().  

The primary reason I pushed it down lower to APR is that both KQueue and
EPoll support 'adding while polling'.  That is, Thread A can be inside a
_poll() and other threads can safely add an FD to the query set.  That
FD will show up in the result set without any other magic. (not possible
with traditional poll/select).

> w.r.t. unlimited size of pollset: this could be solved in a general
> manner, for *all* implementations, by adding an apr_pollset_resize()
> call.

I thought about doing a _resize function originally.  To safely resize,
you would need to lock everything out first.  Since there isn't really a
realloc type function for pools, you would end up palloc`ing new arrays
and copying the data.  This seems really bad to me on a busy server.  It
does not seem very realistic or efficient to basically pause a busy
server and copy 20,000 FDs over to an array that is bigger.

> So I'm worried this patch is really solving the problem in the wrong
> place.  The thing you are exporting here in the APR API, "is the
> apr_pollset_* interface thread-safe in _add and _remove", seems really
> horrible.  Either an API is thread-safe, or it isn't.  Making that a
> *per-platform* flag seems like really bad design.
>
Perhaps.  First thing I am going to do is break up the current code in
CVS into multiple files.  I am really starting to dislike all these
ifdefs in the current implementation.  I think this should be done
regardless of this patch.

-Paul Querna


Mime
View raw message