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
