From dev-return-7013-apmail-apr-dev-archive=apr.apache.org@apr.apache.org Sat Jul 06 09:39:05 2002 Return-Path: Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 30310 invoked by uid 500); 6 Jul 2002 09:39:05 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 30299 invoked from network); 6 Jul 2002 09:39:04 -0000 X-Authentication-Warning: cancer.clove.org: jerenk set sender to jerenkrantz@apache.org using -f Date: Sat, 6 Jul 2002 02:39:18 -0700 From: Justin Erenkrantz To: dev@apr.apache.org Subject: New apr_poll() implementation was Re: [PATCH] speed up network timeout processing Message-ID: <20020706023918.D20377@apache.org> Mail-Followup-To: Justin Erenkrantz , dev@apr.apache.org References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: ; from rbb@apache.org on Fri, Jul 05, 2002 at 08:47:32PM -0400 X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N On Fri, Jul 05, 2002 at 08:47:32PM -0400, rbb@apache.org wrote: > This also creates a support library for APR, this is basically just a > series of functions that APR can use internally to get the job > done. Since wait_for_io_or_timeout was identical between files and > sockets, I have moved that function to the support lib, it also now uses > apr_poll(). What's the rationale behind a 'support library' for APR? All of the content in newpoll.tar.gz seems like it should just stay in APR. You have me thoroughly confused here. Perhaps in a new directory, but definitely not a separate library. I'm also not clear what the additional num parameter to apr_poll is buying us. Why not just use the *nsds value on input as your num parameter? The number of descriptors read is what *nsds is on the output. No signature change required. Aha, it seems that httpd uses a broken apr_poll() semantic. If you read the documentation for apr_poll, it says: * @param nsds The number of sockets we are polling. ... * The number of sockets signalled is returned in the second argument (nsds) But, httpd-2.0 doesn't pass in the number of sockets we are polling. flood does because I read the docs not the code. I think we should switch the code to match the docs. I know I took it to mean that nsds on input is the number of fds we are polling and on completion, it is the number of signaled fds. I believe that the docs have the right API. So, why not do something like this (very rough): (Some OS does the low magic number off the stack, but I can't remember which for the life of me.) apr_status_t apr_poll(apr_pollfd_t *aprset, apr_int32_t *nsds, apr_interval_time_t timeout) { int i, rv; struct pollfd *pollset; struct pollfd stack_pollset[SOME_LOW_MAGIC_NUMBER_LIKE_SAY_6]; if (*nsds < SOME_LOW_MAGIC_NUMBER_LIKE_SAY_6) { pollset = &stack_pollset; } else { pollset = apr_palloc(aprset->p, sizeof(struct pollfd) * *nsds); } for (i = 0; i < *nsds; i++) { if (aprset[i].desc_type == APR_POLL_SOCKET) { pollset[i].fd = aprset[i].desc.s->socketdes; } else if (aprset[i].desc_type == APR_POLL_FILE) { pollset[i].fd = aprset[i].desc.f->filedes; } pollset[i].events = get_event(aprset[i].events); } if (timeout > 0) { /* FIXME: need apr conversion macro from apr_interval_time_t * to milliseconds. */ timeout /= 1000; /* convert microseconds to milliseconds */ } rv = poll(pollset, *nsds, timeout); if (rv < 0) { return errno; } for (i = 0; i < *nsds; i++) { aprset[i].revents = get_revent(pollset[i].revents); } *nsds = rv; return APR_SUCCESS; } It seems that you might have originally meant to go down this route because you have: struct pollfd *pollset = apr_palloc(aprset->p, sizeof(struct pollfd) * *nsds); Which means that the pollset is allocated to contain *nsds pollfds. And, I think that means you need to have *nsds to be equivalent to num, or you will segfault. (Perhaps it hasn't in your tests by sheer luck.) -- justin