Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-11-12 Thread Ryan Bloom

On Monday 12 November 2001 09:48 pm, Justin Erenkrantz wrote:
> On Mon, Nov 12, 2001 at 11:49:08PM -, [EMAIL PROTECTED] wrote:
> > rbb 01/11/12 15:49:08
> >   Log:
> >   Begin to abstract out the underlying transport layer.
> >   The first step is to remove the socket from the conn_rec,
> >   the server now lives in a context that is passed to the
> >   core's input and output filters. This forces us to be very
> >   careful when adding calls that use the socket directly,
> >   because the socket isn't available in most locations.
>
> modules/proxy/proxy_connect.c does raw socket writes (see line
> 308).  I think the idea here is that mod_proxy wants to bypass
> everyone.  Not the greatest of ideas (quite bogus actually) -
> perhaps we can setup a minimal filter output stack.
>
> Thoughts?  I haven't had time to digest or even look at your
> patch yet, so I'm not really sure what you did.  -- justin

The proxy is bogus currently.  However, the part you are talking about
is the client side of the proxy, not the server.  When Apache is acting as
a client, it can use network primitives directly.  The important part of this
patch, is that it is possible to accept requests from transport layers that 
aren't sockets.

Ryan
__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-11-12 Thread Justin Erenkrantz

On Mon, Nov 12, 2001 at 11:49:08PM -, [EMAIL PROTECTED] wrote:
> rbb 01/11/12 15:49:08
>   Log:
>   Begin to abstract out the underlying transport layer.
>   The first step is to remove the socket from the conn_rec,
>   the server now lives in a context that is passed to the
>   core's input and output filters. This forces us to be very
>   careful when adding calls that use the socket directly,
>   because the socket isn't available in most locations.

modules/proxy/proxy_connect.c does raw socket writes (see line 
308).  I think the idea here is that mod_proxy wants to bypass 
everyone.  Not the greatest of ideas (quite bogus actually) - 
perhaps we can setup a minimal filter output stack.  

Thoughts?  I haven't had time to digest or even look at your
patch yet, so I'm not really sure what you did.  -- justin




Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-11-10 Thread Ryan Bloom

On Saturday 10 November 2001 12:58 pm, Bill Stoddard wrote:
> Sorry, my last message wasn't a very useful...
>
> ap_lingering_close() should be conditionally called based on a feature
> macro. Something like: HAVE_REUSE_ACCEPT_SOCKET.  If the OS supports
> reusing the accept socket, conditionally issue ap_lingering_close() if the
> socket has not been disconnected.
>
> I'll fix this when we get the filter architecture straigtened out.
>
> BTW, I have started writing a network_out_filter (spilitting function out
> of core_output_filter). Have limited time to work on it but should have
> something ready to post early next week.

This functionality is VERY MPM dependant, and I can't see a good way to
export the information from the MPM.  So, I am allowing the MPM to kill
the cleanup.

BTW, I have the full patch about half done right now.  This will be the patch
that will allow you to easily abstract out the underlying network.  I am 
continuing to move network functions into a few functions.  I am not tending
to use filters, because the filters have been in the wrong locations.

Ryan
__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-11-10 Thread Ryan Bloom

On Saturday 10 November 2001 12:37 pm, Bill Stoddard wrote:
> Ryan,
> I understand the motivation for this, but it breaks Windows. In some cases
> we do not want to close the socket on Windows; it can be reused for a big
> performance boost.

I'm fixing it.  This is going to require me to re-export the lingering_close
function though, which I really didn't want to do.

:-(

Ryan

__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-11-10 Thread Bill Stoddard

Sorry, my last message wasn't a very useful...

ap_lingering_close() should be conditionally called based on a feature macro. Something
like: HAVE_REUSE_ACCEPT_SOCKET.  If the OS supports reusing the accept socket,
conditionally issue ap_lingering_close() if the socket has not been disconnected.

I'll fix this when we get the filter architecture straigtened out.

BTW, I have started writing a network_out_filter (spilitting function out of
core_output_filter). Have limited time to work on it but should have something ready to
post early next week.

Bill

- Original Message -
From: "Bill Stoddard" <[EMAIL PROTECTED]>
To: <[EMAIL PROTECTED]>
Sent: Saturday, November 10, 2001 3:37 PM
Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c


> Ryan,
> I understand the motivation for this, but it breaks Windows. In some cases we do not
want
> to close the socket on Windows; it can be reused for a big performance boost.
>
> Bill
>
> - Original Message -
> From: <[EMAIL PROTECTED]>
> To: <[EMAIL PROTECTED]>
> Sent: Saturday, November 10, 2001 1:26 PM
> Subject: cvs commit: httpd-2.0/server/mpm/worker worker.c
>
>
> > rbb 01/11/10 10:26:30
> >
> >   Modified:include  http_connection.h
> >server   connection.c
> >server/mpm/beos beos.c
> >server/mpm/mpmt_os2 mpmt_os2_child.c
> >server/mpm/netware mpm_netware.c
> >server/mpm/perchild perchild.c
> >server/mpm/prefork prefork.c
> >server/mpm/spmt_os2 spmt_os2.c
> >server/mpm/threaded threaded.c
> >server/mpm/winnt mpm_winnt.c
> >server/mpm/worker worker.c
> >   Log:
> >   Remove ap_lingering_close from all of the MPMs. This is now done as
> >   a cleanup registered with the connection_pool.  I have also turned
> >   ap_lingering_close into a static function, because it is only used
> >   in connection.c.  This is the next step to consolidating all of the
> >   socket function calls.  ap_lingering_close will only be added if the
> >   core is dealing with a standard socket.
> >
> >   Revision  ChangesPath
> >   1.38  +0 -16 httpd-2.0/include/http_connection.h
> >
> >   Index: http_connection.h
> >   ===
> >   RCS file: /home/cvs/httpd-2.0/include/http_connection.h,v
> >   retrieving revision 1.37
> >   retrieving revision 1.38
> >   diff -u -r1.37 -r1.38
> >   --- http_connection.h 2001/07/27 21:01:16 1.37
> >   +++ http_connection.h 2001/11/10 18:26:29 1.38
> >   @@ -88,22 +88,6 @@
> >
> >AP_CORE_DECLARE(void) ap_flush_conn(conn_rec *c);
> >
> >   -/**
> >   - * This function is responsible for the following cases:
> >   - * 
> >   - * we now proceed to read from the client until we get EOF, or until
> >   - * MAX_SECS_TO_LINGER has passed.  the reasons for doing this are
> >   - * documented in a draft:
> >   - *
> >   - * http://www.ics.uci.edu/pub/ietf/http/draft-ietf-http-connection-00.txt
> >   - *
> >   - * in a nutshell -- if we don't make this effort we risk causing
> >   - * TCP RST packets to be sent which can tear down a connection before
> >   - * all the response data has been sent to the client.
> >   - * 
> >   - * @param c The connection we are closing
> >   - */
> >   -void ap_lingering_close(conn_rec *);
> >#endif
> >
> >  /* Hooks */
> >
> >
> >
> >   1.85  +7 -3  httpd-2.0/server/connection.c
> >
> >   Index: connection.c
> >   ===
> >   RCS file: /home/cvs/httpd-2.0/server/connection.c,v
> >   retrieving revision 1.84
> >   retrieving revision 1.85
> >   diff -u -r1.84 -r1.85
> >   --- connection.c 2001/07/31 00:34:27 1.84
> >   +++ connection.c 2001/11/10 18:26:29 1.85
> >   @@ -149,13 +149,14 @@
> > * all the response data has been sent to the client.
> > */
> >#define SECONDS_TO_LINGER  2
> >   -void ap_lingering_close(conn_rec *c)
> >   +static apr_status_t ap_lingering_close(void *dummy)
> >{
> >char dummybuf[512];
> >apr_size_t nbytes = sizeof(dummybuf);
> >apr_status_t rc;
> >apr_int32_t timeout;
> >apr_int32_t total_linger_time = 0;
> >   +conn_rec *c = dummy;
> >
> >ap_update_child_status(AP_CHILD_THREAD_FROM_ID(c->id), SERVER_CLOSING, 
>N

Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-11-10 Thread Bill Stoddard

Ryan,
I understand the motivation for this, but it breaks Windows. In some cases we do not 
want
to close the socket on Windows; it can be reused for a big performance boost.

Bill

- Original Message -
From: <[EMAIL PROTECTED]>
To: <[EMAIL PROTECTED]>
Sent: Saturday, November 10, 2001 1:26 PM
Subject: cvs commit: httpd-2.0/server/mpm/worker worker.c


> rbb 01/11/10 10:26:30
>
>   Modified:include  http_connection.h
>server   connection.c
>server/mpm/beos beos.c
>server/mpm/mpmt_os2 mpmt_os2_child.c
>server/mpm/netware mpm_netware.c
>server/mpm/perchild perchild.c
>server/mpm/prefork prefork.c
>server/mpm/spmt_os2 spmt_os2.c
>server/mpm/threaded threaded.c
>server/mpm/winnt mpm_winnt.c
>server/mpm/worker worker.c
>   Log:
>   Remove ap_lingering_close from all of the MPMs. This is now done as
>   a cleanup registered with the connection_pool.  I have also turned
>   ap_lingering_close into a static function, because it is only used
>   in connection.c.  This is the next step to consolidating all of the
>   socket function calls.  ap_lingering_close will only be added if the
>   core is dealing with a standard socket.
>
>   Revision  ChangesPath
>   1.38  +0 -16 httpd-2.0/include/http_connection.h
>
>   Index: http_connection.h
>   ===
>   RCS file: /home/cvs/httpd-2.0/include/http_connection.h,v
>   retrieving revision 1.37
>   retrieving revision 1.38
>   diff -u -r1.37 -r1.38
>   --- http_connection.h 2001/07/27 21:01:16 1.37
>   +++ http_connection.h 2001/11/10 18:26:29 1.38
>   @@ -88,22 +88,6 @@
>
>AP_CORE_DECLARE(void) ap_flush_conn(conn_rec *c);
>
>   -/**
>   - * This function is responsible for the following cases:
>   - * 
>   - * we now proceed to read from the client until we get EOF, or until
>   - * MAX_SECS_TO_LINGER has passed.  the reasons for doing this are
>   - * documented in a draft:
>   - *
>   - * http://www.ics.uci.edu/pub/ietf/http/draft-ietf-http-connection-00.txt
>   - *
>   - * in a nutshell -- if we don't make this effort we risk causing
>   - * TCP RST packets to be sent which can tear down a connection before
>   - * all the response data has been sent to the client.
>   - * 
>   - * @param c The connection we are closing
>   - */
>   -void ap_lingering_close(conn_rec *);
>#endif
>
>  /* Hooks */
>
>
>
>   1.85  +7 -3  httpd-2.0/server/connection.c
>
>   Index: connection.c
>   ===
>   RCS file: /home/cvs/httpd-2.0/server/connection.c,v
>   retrieving revision 1.84
>   retrieving revision 1.85
>   diff -u -r1.84 -r1.85
>   --- connection.c 2001/07/31 00:34:27 1.84
>   +++ connection.c 2001/11/10 18:26:29 1.85
>   @@ -149,13 +149,14 @@
> * all the response data has been sent to the client.
> */
>#define SECONDS_TO_LINGER  2
>   -void ap_lingering_close(conn_rec *c)
>   +static apr_status_t ap_lingering_close(void *dummy)
>{
>char dummybuf[512];
>apr_size_t nbytes = sizeof(dummybuf);
>apr_status_t rc;
>apr_int32_t timeout;
>apr_int32_t total_linger_time = 0;
>   +conn_rec *c = dummy;
>
>ap_update_child_status(AP_CHILD_THREAD_FROM_ID(c->id), SERVER_CLOSING, NULL);
>
>   @@ -175,7 +176,7 @@
>
>if (c->aborted) {
>apr_socket_close(c->client_socket);
>   -return;
>   +return APR_SUCCESS;
>}
>
>/* Shut down the socket for write, which will send a FIN
>   @@ -185,7 +186,7 @@
>if (apr_shutdown(c->client_socket, APR_SHUTDOWN_WRITE) != APR_SUCCESS ||
>c->aborted) {
>apr_socket_close(c->client_socket);
>   -return;
>   +return APR_SUCCESS;
>}
>
>/* Read all data from the peer until we reach "end-of-file" (FIN
>   @@ -208,6 +209,7 @@
>}
>
>apr_socket_close(c->client_socket);
>   +return APR_SUCCESS;
>}
>
>AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c)
>   @@ -261,6 +263,8 @@
>conn->client_socket = inout;
>
>conn->id = id;
>   +
>   +apr_pool_cleanup_register(p, conn, ap_lingering_close, apr_pool_cleanup_null);
>
>return conn;
>}
>
>
>
>   1.66  +0 -1  httpd-2.0/server/mpm/beos/beos.c
>
>   Index: beos.c
>   ===
>   RCS file: /home/cvs/httpd-2.0/server/mpm/beos/beos.c,v
>   retrieving revision 1.65
>   retrieving revision 1.66
>   diff -u -r1.65 -r1.66
>   --- beos.c 2001/11/08 22:49:12 1.65
>   +++ beos.c 2001/11/10 18:26:29 1.66
>   @@ -316,7 +316,6 @@
>
>if (current_conn) {
>ap_process_connection(current_conn);
>   -ap_lingering_close(current_conn);
>}
>}
>
>
>
>
>   1.3   +0 -1  http

Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-20 Thread Aaron Bannert

On Thu, Sep 20, 2001 at 01:04:48AM -0700, Justin Erenkrantz wrote:
> On Thu, Sep 20, 2001 at 01:00:09AM -0700, Greg Stein wrote:
> > >...
> > > Whoever does the software behind apache-mbox (I take it this is 
> > > mod_mbox?) might want to take note that it's spitting out invalid URLs..
> > 
> > The URLs produced by mod_mbox are fine. Aaron must have posted an unescaped
> > version of the URL.
> 
> I have a feeling Aaron manually generated the URL.  -- justin

Huh? No way, that was the one from my Location: bar in Netscape.

-aaron




Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-20 Thread Justin Erenkrantz

On Thu, Sep 20, 2001 at 01:00:09AM -0700, Greg Stein wrote:
> >...
> > Whoever does the software behind apache-mbox (I take it this is 
> > mod_mbox?) might want to take note that it's spitting out invalid URLs..
> 
> The URLs produced by mod_mbox are fine. Aaron must have posted an unescaped
> version of the URL.

I have a feeling Aaron manually generated the URL.  -- justin




Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-20 Thread Greg Stein

On Thu, Sep 20, 2001 at 12:53:39AM -0700, Alex Stewart wrote:
> On a largely unrelated note, but something I found a little ironic given 
> the nature of this list:
> 
> Aaron Bannert wrote:
> 
> > 
>http://www.apachelabs.org/apache-mbox/199902.mbox/<[EMAIL PROTECTED]>
> 
> Please note that the above is not a valid URL.  Specifically, the "<"

Agreed.

>...
> Whoever does the software behind apache-mbox (I take it this is 
> mod_mbox?) might want to take note that it's spitting out invalid URLs..

The URLs produced by mod_mbox are fine. Aaron must have posted an unescaped
version of the URL.

(go to a mod_mbox page and view the source...)

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-20 Thread Alex Stewart

On a largely unrelated note, but something I found a little ironic given 
the nature of this list:

Aaron Bannert wrote:

> http://www.apachelabs.org/apache-mbox/199902.mbox/<[EMAIL PROTECTED]>


Please note that the above is not a valid URL.  Specifically, the "<" 
and ">" characters are technically not allowed in URLs and must be 
escaped.  (I bring this up partly because in Mozilla, this message came 
up with two links, a http: link to 199902.mbox, and a mailto: link to 
[EMAIL PROTECTED], so I had to do some cutting and 
pasting to actually see the right document)

Whoever does the software behind apache-mbox (I take it this is 
mod_mbox?) might want to take note that it's spitting out invalid URLs..

-alex








Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Aaron Bannert

On Wed, Sep 19, 2001 at 02:10:06PM -0700, Ryan Bloom wrote:
> Take a look at:
> 
> http://cvs.apache.org/viewcvs.cgi/apache-apr/pthreads/src/main/fdqueue.c
> 
> specifically at revision 1.12, where I committed code to have
> 
> slots in queue == Threads per child.
> 
> This would be the time frame to review the archives.

I've been skimming the archives, but I came across one you should
all get a kick out of:

http://www.apachelabs.org/apache-mbox/199902.mbox/<[EMAIL PROTECTED]>

-aaron



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Ryan Bloom

On Wednesday 19 September 2001 02:32 pm, Aaron Bannert wrote:
> On Wed, Sep 19, 2001 at 02:03:13PM -0700, Ryan Bloom wrote:
> > On Wednesday 19 September 2001 01:49 pm, Aaron Bannert wrote:
> > > I'm not sure if simply closing the socket is the right answer either.
> > > Is there a more appropriate error code? "Server Temporarily
> > > Unavailable" or whatnot?
> > >
> > > I'd agree that we can never know if the server is truly overloaded,
> > > but with a sufficiently long queue and an assumed random distribution
> > > of requests to the children, I'd say "Server Busy" is pretty close to
> > > the truth. It is definately the truth for that particular child.
> >
> > You shouldn't ever be in this situation.  This is why the queue_full
> > condition existed to begin with.  Basically, the listener should stop
> > listening if it doesn't have enough workers to handle a connection.  If
> > we are putting requests in the queue without a worker to handle it
> > immediately, then we take the very real chance of starving that request.
>
> Well, in that case I think I'll have to put that other condition back in
> again. I see now why it was necessary.
>
> But isn't it true that even with the not_full condition, you can be
> in a state where each worker is off servicing a request, and your
> queue is full (The listener is blocking on that not_full condition).
> Let's say that at that point the server does a graceful restart.
> The workers will happily finish their connections, but they will
> exit their loop and die before checking the queue. That is the same
> situation we're in right now, where it is possible for N already
> accepted sockets to be the queue with no workers to service them.
> (I trust you'll correct me if I'm wrong here)

The workers should check the queue before they die.  If they do that,
then we won't drop any connections at all.

> > > shorter queue ==> lower first-byte latency, less sockets mercilessly
> > > killed, less tolerant to request spikes.
> > >
> > > I doubt it will have a significant enough impact in the general case to
> > > warrant a runtime parameter, but I'll definately consider a
> > > compile-time definition (or at least something to override the default
> > > of
> > > threads_per_child).
> >
> > Manoj and I originally thought that we could add a few extra spots to the
> > queue, so that as a worker finished, there was something for it to grab
> > and work on immediately.  In Manoj's tests however, this ended up not
> > really helping us at all.
>
> The fast-path comes from when the worker thread checks the queue for
> non-emptyness. Consider a server where all workers are off servicing
> requests. At first the queue is empty, but as it fills up and workers
> return, they do not need to call cond_wait(), since there already is
> a socket waiting for them.

Agreed that this is the goal, but the problem is the potential starvation
when all threads are serving long-lived requests.

Ryan

__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Aaron Bannert

On Wed, Sep 19, 2001 at 02:03:13PM -0700, Ryan Bloom wrote:
> On Wednesday 19 September 2001 01:49 pm, Aaron Bannert wrote:
> > I'm not sure if simply closing the socket is the right answer either.
> > Is there a more appropriate error code? "Server Temporarily Unavailable"
> > or whatnot?
> >
> > I'd agree that we can never know if the server is truly overloaded,
> > but with a sufficiently long queue and an assumed random distribution
> > of requests to the children, I'd say "Server Busy" is pretty close to
> > the truth. It is definately the truth for that particular child.
> 
> You shouldn't ever be in this situation.  This is why the queue_full condition
> existed to begin with.  Basically, the listener should stop listening if it doesn't
> have enough workers to handle a connection.  If we are putting requests in
> the queue without a worker to handle it immediately, then we take the very real
> chance of starving that request.

Well, in that case I think I'll have to put that other condition back in
again. I see now why it was necessary.

But isn't it true that even with the not_full condition, you can be
in a state where each worker is off servicing a request, and your
queue is full (The listener is blocking on that not_full condition).
Let's say that at that point the server does a graceful restart.
The workers will happily finish their connections, but they will
exit their loop and die before checking the queue. That is the same
situation we're in right now, where it is possible for N already
accepted sockets to be the queue with no workers to service them.
(I trust you'll correct me if I'm wrong here)

> > longer queue ==> higher first-byte latency, more accepted sockets
> > spontaneously die when we do a graceful restart. better tolerance to
> > request spikes.
> 
> We should NEVER kill a request during a graceful.  This problem goes
> away if we stop accepting connections when we don't have a worker to
> handle it.

Agreed.

> > shorter queue ==> lower first-byte latency, less sockets mercilessly
> > killed, less tolerant to request spikes.
> >
> > I doubt it will have a significant enough impact in the general case to
> > warrant a runtime parameter, but I'll definately consider a compile-time
> > definition (or at least something to override the default of
> > threads_per_child).
> 
> Manoj and I originally thought that we could add a few extra spots to the
> queue, so that as a worker finished, there was something for it to grab and
> work on immediately.  In Manoj's tests however, this ended up not really 
> helping us at all.

The fast-path comes from when the worker thread checks the queue for
non-emptyness. Consider a server where all workers are off servicing
requests. At first the queue is empty, but as it fills up and workers
return, they do not need to call cond_wait(), since there already is
a socket waiting for them.

-aaron



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Ryan Bloom

On Wednesday 19 September 2001 02:05 pm, Ryan Bloom wrote:
> On Wednesday 19 September 2001 01:59 pm, Jeff Trawick wrote:
> > Ryan Bloom <[EMAIL PROTECTED]> writes:
> > > On Wednesday 19 September 2001 01:21 pm, Greg Ames wrote:
> > > > For now, ThreadsPerChild might be fine.  I'd feel a little better
> > > > about some small constant, like 5 or 10.  But the code does need to
> > > > learn to deal with "queue full" more gracefully.
> > >
> > > Guys, please take a look at the archives.  Manoj did these tests over a
> > > year ago.  ThreadsPerChild is the optimal value.  We have run the
> > > numbers once already.
> >
> > I would think that tests for reasonable queue limit need to be done
> > with an MPM hich is otherwise working fairly reasonably (e.g., not
> > losing connections when the queue fills).  Surely we haven't been
> > spinning our wheels all this time when Manoj had one lying around :)
>
> The tests were done before the MPM logic was created.  And yes, the worker
> MPM does go back to the very first design that Manoj and I wrote in 6
> months while we were both back at IBM.  Why do you think I keep saying that
> we are constantly re-visiting decisions that were made in the past for good
> reasons.
>
> The worker MPM is not new, the fdqueue logic is not new.  The design comes
> from the Lotus Domino Go WebServer, and the code was written over three
> years ago.

Take a look at:

http://cvs.apache.org/viewcvs.cgi/apache-apr/pthreads/src/main/fdqueue.c

specifically at revision 1.12, where I committed code to have

slots in queue == Threads per child.

This would be the time frame to review the archives.

Ryan


__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Ryan Bloom

On Wednesday 19 September 2001 01:59 pm, Jeff Trawick wrote:
> Ryan Bloom <[EMAIL PROTECTED]> writes:
> > On Wednesday 19 September 2001 01:21 pm, Greg Ames wrote:
> > > For now, ThreadsPerChild might be fine.  I'd feel a little better about
> > > some small constant, like 5 or 10.  But the code does need to learn to
> > > deal with "queue full" more gracefully.
> >
> > Guys, please take a look at the archives.  Manoj did these tests over a
> > year ago.  ThreadsPerChild is the optimal value.  We have run the numbers
> > once already.
>
> I would think that tests for reasonable queue limit need to be done
> with an MPM hich is otherwise working fairly reasonably (e.g., not
> losing connections when the queue fills).  Surely we haven't been
> spinning our wheels all this time when Manoj had one lying around :)

The tests were done before the MPM logic was created.  And yes, the worker
MPM does go back to the very first design that Manoj and I wrote in 6 months
while we were both back at IBM.  Why do you think I keep saying that we are
constantly re-visiting decisions that were made in the past for good reasons.

The worker MPM is not new, the fdqueue logic is not new.  The design comes
from the Lotus Domino Go WebServer, and the code was written over three years
ago.

Ryan

__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Ryan Bloom

On Wednesday 19 September 2001 01:49 pm, Aaron Bannert wrote:
> On Wed, Sep 19, 2001 at 12:18:31PM -0700, Ryan Bloom wrote:
> > On Wednesday 19 September 2001 12:11 pm, Aaron Bannert wrote:
> > > So I have two questions:
> > >
> > > 1) How do we send back that error?
> >
> > You can't.  There is no way to determine how busy the other processes
> > are.
>
> I'm not sure if simply closing the socket is the right answer either.
> Is there a more appropriate error code? "Server Temporarily Unavailable"
> or whatnot?
>
> I'd agree that we can never know if the server is truly overloaded,
> but with a sufficiently long queue and an assumed random distribution
> of requests to the children, I'd say "Server Busy" is pretty close to
> the truth. It is definately the truth for that particular child.

You shouldn't ever be in this situation.  This is why the queue_full condition
existed to begin with.  Basically, the listener should stop listening if it doesn't
have enough workers to handle a connection.  If we are putting requests in
the queue without a worker to handle it immediately, then we take the very real
chance of starving that request.

> > > 2) How long should the queue be? Should we just set some arbitrary
> > > constant, defined in mpm_default.h, or should we come up with some
> > > heuristic?
> >
> > Manoj actually ran tests about this back when we were using the
> > apache-apr repository.  In his tests, the optimal length was equal to the
> > number of threads. Having a bit of wiggle room just didn't get us
> > anywhere.
>
> Sounds reasonable. It's just a tradeoff:
>
> longer queue ==> higher first-byte latency, more accepted sockets
> spontaneously die when we do a graceful restart. better tolerance to
> request spikes.

We should NEVER kill a request during a graceful.  This problem goes
away if we stop accepting connections when we don't have a worker to
handle it.

> shorter queue ==> lower first-byte latency, less sockets mercilessly
> killed, less tolerant to request spikes.
>
> I doubt it will have a significant enough impact in the general case to
> warrant a runtime parameter, but I'll definately consider a compile-time
> definition (or at least something to override the default of
> threads_per_child).

Manoj and I originally thought that we could add a few extra spots to the
queue, so that as a worker finished, there was something for it to grab and
work on immediately.  In Manoj's tests however, this ended up not really 
helping us at all.

Ryan
__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Jeff Trawick

Aaron Bannert <[EMAIL PROTECTED]> writes:

> On Wed, Sep 19, 2001 at 12:18:31PM -0700, Ryan Bloom wrote:
> > On Wednesday 19 September 2001 12:11 pm, Aaron Bannert wrote:
> > > So I have two questions:
> > >
> > > 1) How do we send back that error?
> > 
> > You can't.  There is no way to determine how busy the other processes are.
> 
> I'm not sure if simply closing the socket is the right answer
> either.

It certainly isn't right :)  Don't call accept() if you can't handle
another connection.

> Is there a more appropriate error code? "Server Temporarily Unavailable"
> or whatnot?

It is an avoidable problem.  There is no sense worrying about the error
code.

-- 
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
   http://www.geocities.com/SiliconValley/Park/9289/
 Born in Roswell... married an alien...



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Jeff Trawick

Ryan Bloom <[EMAIL PROTECTED]> writes:

> On Wednesday 19 September 2001 01:21 pm, Greg Ames wrote:
> > For now, ThreadsPerChild might be fine.  I'd feel a little better about
> > some small constant, like 5 or 10.  But the code does need to learn to
> > deal with "queue full" more gracefully.
> 
> Guys, please take a look at the archives.  Manoj did these tests over a year
> ago.  ThreadsPerChild is the optimal value.  We have run the numbers once
> already.

I would think that tests for reasonable queue limit need to be done
with an MPM hich is otherwise working fairly reasonably (e.g., not
losing connections when the queue fills).  Surely we haven't been
spinning our wheels all this time when Manoj had one lying around :)

-- 
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
   http://www.geocities.com/SiliconValley/Park/9289/
 Born in Roswell... married an alien...



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Aaron Bannert

On Wed, Sep 19, 2001 at 12:18:31PM -0700, Ryan Bloom wrote:
> On Wednesday 19 September 2001 12:11 pm, Aaron Bannert wrote:
> > So I have two questions:
> >
> > 1) How do we send back that error?
> 
> You can't.  There is no way to determine how busy the other processes are.

I'm not sure if simply closing the socket is the right answer either.
Is there a more appropriate error code? "Server Temporarily Unavailable"
or whatnot?

I'd agree that we can never know if the server is truly overloaded,
but with a sufficiently long queue and an assumed random distribution
of requests to the children, I'd say "Server Busy" is pretty close to
the truth. It is definately the truth for that particular child.

> > 2) How long should the queue be? Should we just set some arbitrary
> > constant, defined in mpm_default.h, or should we come up with some
> > heuristic?
> 
> Manoj actually ran tests about this back when we were using the apache-apr
> repository.  In his tests, the optimal length was equal to the number of threads.
> Having a bit of wiggle room just didn't get us anywhere.

Sounds reasonable. It's just a tradeoff:

longer queue ==> higher first-byte latency, more accepted sockets spontaneously
 die when we do a graceful restart. better tolerance to
 request spikes.

shorter queue ==> lower first-byte latency, less sockets mercilessly killed,
 less tolerant to request spikes.

I doubt it will have a significant enough impact in the general case to warrant
a runtime parameter, but I'll definately consider a compile-time definition
(or at least something to override the default of threads_per_child).

-aaron



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Ryan Bloom

On Wednesday 19 September 2001 01:21 pm, Greg Ames wrote:
> Aaron Bannert wrote:
> > 2) How long should the queue be? Should we just set some arbitrary
> > constant, defined in mpm_default.h, or should we come up with some
> > heuristic?
>
> Ideally, you would have 0 connections on the queue most of the time.
> Then, just before some worker gets done with its last connection, 1 new
> connection briefly appears in the queue.  That gives you the minimum
> latency without a lot of blocking & unblocking overhead.  Of course you
> can't control the incoming connection rate in the real world, but you
> can control the number of threads which are actively trying to consume
> from the queue.
>
> For now, ThreadsPerChild might be fine.  I'd feel a little better about
> some small constant, like 5 or 10.  But the code does need to learn to
> deal with "queue full" more gracefully.

Guys, please take a look at the archives.  Manoj did these tests over a year
ago.  ThreadsPerChild is the optimal value.  We have run the numbers once
already.

Ryan
__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Greg Ames

Aaron Bannert wrote:
> 
> 2) How long should the queue be? Should we just set some arbitrary constant,
>defined in mpm_default.h, or should we come up with some heuristic?

Ideally, you would have 0 connections on the queue most of the time. 
Then, just before some worker gets done with its last connection, 1 new
connection briefly appears in the queue.  That gives you the minimum
latency without a lot of blocking & unblocking overhead.  Of course you
can't control the incoming connection rate in the real world, but you
can control the number of threads which are actively trying to consume
from the queue.

For now, ThreadsPerChild might be fine.  I'd feel a little better about
some small constant, like 5 or 10.  But the code does need to learn to
deal with "queue full" more gracefully.

Greg



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Jeff Trawick

[EMAIL PROTECTED] writes:


>   +++ worker.c2001/09/19 18:47:31 1.26
>   @@ -659,7 +659,16 @@
>signal_workers();
>}
>if (csd != NULL) {
>   -ap_queue_push(worker_queue, csd, ptrans);
>   +rv = ap_queue_push(worker_queue, csd, ptrans);
>   +if (rv) {
>   +/* trash the connection; we couldn't queue the connected
>   + * socket to a worker 
>   + */
>   +apr_socket_close(csd);
>   +ap_log_error(APLOG_MARK, APLOG_CRIT, 0, ap_server_conf,
>   + "ap_queue_push failed with error code %d",
>   + rv);

While playing with worker today I also had a call to ap_log_error()
for ap_queue_pop() failures, but I was immediately reminded of
experiences with my own similar MPM, which showed that there are false
wake-ups with glibc 2.1 on Linux.

For ap_queue_pop(), we could stand to have a call to ap_log_error()
when it fails with something besides FD_QUEUE_EINTR.

-- 
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
   http://www.geocities.com/SiliconValley/Park/9289/
 Born in Roswell... married an alien...



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Jeff Trawick

Aaron Bannert <[EMAIL PROTECTED]> writes:

> On Wed, Sep 19, 2001 at 06:47:31PM -, [EMAIL PROTECTED] wrote:
> > trawick 01/09/19 11:47:31
> > 
> >   Modified:server/mpm/worker worker.c
> >   Log:
> >   if we're gonna trash the connection due to a queue overflow, at the
> >   very least we should close the socket and write a log message (mostly
> >   to aid debugging, as this is a showstopper problem)
> >   
> >   this is no fix; there is a design issue to consider; hopefully this
> >   will
> 
> [I assume you had more to say?]

I forgot what the rest of the sentence was :)  I intended to follow up
here anyway, but in my rush home to beat the school bus you chimed in
first.

> Now that the queue represents the number of accepted-but-not-processed
> sockets, it does not necessarily need to be the size of the number of
> threads, but instead some other value that indicates the number of
> sockets we'll accept before sending back some "Server Busy" error.
> 
> So I have two questions:
> 
> 1) How do we send back that error?
> 
> 2) How long should the queue be? Should we just set some arbitrary constant,
>defined in mpm_default.h, or should we come up with some heuristic?

Let's pick a number for now for the length of the queue.  As far as I
am concerned, the current number is a reasonable starting point.

It shouldn't be too low, since when the server is busy we want to
always have some available work when workers are done processing a
previous connection.

It shouldn't be too high because

1) the queue may be full because our workers are busy, and for all we 
   know there is another httpd process with workers which aren't busy,
   and we shouldn't be accepting too many new connections because that
   keeps the idle workers in another process from processing them;
   with a larger queue we have higher risk of starving workers in
   another process even though we can't do anything ourselves
2) (not terribly important to me) any connection sitting in the queue
   at graceful restart time is a connection which could have been
   processed with the new configuration but won't be since we were
   needlessly greedy

If we avoid calling accept when our queue is full, we should fix this
issue.  

So what to do?

query state of queue before accept (without getting a lock if
possible)...  if pretty full*, instead of calling accept() block in a mutex
which will be posted by a worker when get go above some threshhold of
available slots in the queue

*maybe some lack of exactness here can allow us to avoid a lock
-- 
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
   http://www.geocities.com/SiliconValley/Park/9289/
 Born in Roswell... married an alien...



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Ryan Bloom

On Wednesday 19 September 2001 12:11 pm, Aaron Bannert wrote:
> On Wed, Sep 19, 2001 at 06:47:31PM -, [EMAIL PROTECTED] wrote:
> > trawick 01/09/19 11:47:31
> >
> >   Modified:server/mpm/worker worker.c
> >   Log:
> >   if we're gonna trash the connection due to a queue overflow, at the
> >   very least we should close the socket and write a log message (mostly
> >   to aid debugging, as this is a showstopper problem)
> >
> >   this is no fix; there is a design issue to consider; hopefully this
> >   will
>
> [I assume you had more to say?]
>
> Yes, you are absolutely correct. I am so glad you caught that. This
> reveals another design issue:
>
> Now that the queue represents the number of accepted-but-not-processed
> sockets, it does not necessarily need to be the size of the number of
> threads, but instead some other value that indicates the number of
> sockets we'll accept before sending back some "Server Busy" error.
>
> So I have two questions:
>
> 1) How do we send back that error?

You can't.  There is no way to determine how busy the other processes are.

> 2) How long should the queue be? Should we just set some arbitrary
> constant, defined in mpm_default.h, or should we come up with some
> heuristic?

Manoj actually ran tests about this back when we were using the apache-apr
repository.  In his tests, the optimal length was equal to the number of threads.
Having a bit of wiggle room just didn't get us anywhere.

Ryan

__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Aaron Bannert

On Wed, Sep 19, 2001 at 06:47:31PM -, [EMAIL PROTECTED] wrote:
> trawick 01/09/19 11:47:31
> 
>   Modified:server/mpm/worker worker.c
>   Log:
>   if we're gonna trash the connection due to a queue overflow, at the
>   very least we should close the socket and write a log message (mostly
>   to aid debugging, as this is a showstopper problem)
>   
>   this is no fix; there is a design issue to consider; hopefully this
>   will

[I assume you had more to say?]

Yes, you are absolutely correct. I am so glad you caught that. This
reveals another design issue:

Now that the queue represents the number of accepted-but-not-processed
sockets, it does not necessarily need to be the size of the number of
threads, but instead some other value that indicates the number of
sockets we'll accept before sending back some "Server Busy" error.

So I have two questions:

1) How do we send back that error?

2) How long should the queue be? Should we just set some arbitrary constant,
   defined in mpm_default.h, or should we come up with some heuristic?

-aaron

>   
>   Revision  ChangesPath
>   1.26  +10 -1 httpd-2.0/server/mpm/worker/worker.c
>   
>   Index: worker.c
>   ===
>   RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
>   retrieving revision 1.25
>   retrieving revision 1.26
>   diff -u -r1.25 -r1.26
>   --- worker.c2001/09/19 06:34:11 1.25
>   +++ worker.c2001/09/19 18:47:31 1.26
>   @@ -659,7 +659,16 @@
>signal_workers();
>}
>if (csd != NULL) {
>   -ap_queue_push(worker_queue, csd, ptrans);
>   +rv = ap_queue_push(worker_queue, csd, ptrans);
>   +if (rv) {
>   +/* trash the connection; we couldn't queue the connected
>   + * socket to a worker 
>   + */
>   +apr_socket_close(csd);
>   +ap_log_error(APLOG_MARK, APLOG_CRIT, 0, ap_server_conf,
>   + "ap_queue_push failed with error code %d",
>   + rv);
>   +}
>}
>}
>else {
>   
>   
>   



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Cliff Woolley


[moving this back on-list]


On Wed, 19 Sep 2001, Aaron Bannert wrote:
> On Wed, Sep 19, 2001 at 11:45:57AM -0400, Cliff Woolley wrote:
> > Anyway, I'm of mixed feelings about the (void) thing, since sometimes we
> > do in fact get bugs from having thrown away a return value that we
> > shouldn't have thrown away.  But then again having the (void) in there
> > doesn't make it any more obvious that THAT's where the bug is, I think...
> > who knows.
>
> I could really go either way. I've become more accustomed to keeping
> the (void), for readability/maintainability. Just out of curiosity,
> how would a deliberate (void) reveal bugs? I'm not seeing it.

It wouldn't, that's my point.  We often get bugs from discarding an error
that got returned by some function.  Having the (void) in there makes it
more clear that we're discarding a return value to someone who doesn't
know the function returns a value at all, but it doesn't really help find
the bug.  Why?  You see some bug, so you go into the debugger.  You start
stepping through code.  You see an error condition happen in some
function.  You see that error get ignored by the caller.  That's where
your bug is.  The fact that the caller said (void) doesn't help you at
all.

Oh well, I don't care that much.  I don't think the (request_rec *)NULL
needs to be in there (and I doubt anyone else in the group does either),
but I could be convinced either way on the (void) thing.  If someone feels
strongly about it, just give me a -1 and I'll put the (void)'s back.

--Cliff

--
   Cliff Woolley
   [EMAIL PROTECTED]
   Charlottesville, VA





Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Bill Stoddard


> Bill Stoddard wrote:
> >
> > It's obvious we are not using the return either way.
>
> But without the cast, someone not conversant with the details
> won't know it normally returns a value; they might think it's
> a void function.
>
> > Less is better so dump the cast :-)
>
> I think the cast improves understanding, and it doesn't
> make any difference at the object-code level, so I say 'keep
> it.' :-)

I can go either way. My last bits on the subject

Should someone be interested in using the return code, they would look at the function 
and
discover that it -does- produce a return code and from that point on, they will know 
this
bit of knowledge. For everyone else, not having to look at (void) all over the place
represents a slight reduction in code complexity and a slight improvement in code
readability.

Bill




Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Rodent of Unusual Size

Bill Stoddard wrote:
> 
> It's obvious we are not using the return either way.

But without the cast, someone not conversant with the details
won't know it normally returns a value; they might think it's
a void function.

> Less is better so dump the cast :-)

I think the cast improves understanding, and it doesn't
make any difference at the object-code level, so I say 'keep
it.' :-)
-- 
#kenP-)}

Ken Coar, Sanagendamgagwedweinini  http://Golux.Com/coar/
Author, developer, opinionist  http://Apache-Server.Com/

"All right everyone!  Step away from the glowing hamburger!"



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Bill Stoddard


> On Wed, Sep 19, 2001 at 06:34:11AM -, [EMAIL PROTECTED] wrote:
> > jwoolley01/09/18 23:34:11
> > 
> >   Modified:server/mpm/worker worker.c
> >   Log:
> >   I was kinda hoping those (void)some_function() and (request_rec *)NULL
> >   casts would go away before this committed, but alas I didn't say anything.
> >   :-)  This gets rid of them and a few others just like them that I also
> >   found in worker.c.
> 
> I don't know what the groupthink is on this, or if there is a precedent
> set, but I think the (void)s make it obvious to the reader that we
> are deliberately throwing away the return value.
> 
> As for the (request_rec *)NULL thing, either way is fine for me.
> 
> -aaron

It's obvious we are not using the return either way. Less is better so dump the cast 
:-)

Bill




Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Greg Stein

On Wed, Sep 19, 2001 at 06:34:11AM -, [EMAIL PROTECTED] wrote:
> jwoolley01/09/18 23:34:11
> 
>   Modified:server/mpm/worker worker.c
>   Log:
>   I was kinda hoping those (void)some_function() and (request_rec *)NULL
>   casts would go away before this committed, but alas I didn't say anything.
>   :-)  This gets rid of them and a few others just like them that I also
>   found in worker.c.

Um. I'm leaning towards a -1 on removing those (void) casts. The functions
return a value, and it is not being used. The (void) makes it clear that the
values are being discarded.

And generally, a discarded value isn't a Good Thing, so those (void) casts
leave markers for somebody to fix the code at some future time.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-09-19 Thread Aaron Bannert

On Wed, Sep 19, 2001 at 06:34:11AM -, [EMAIL PROTECTED] wrote:
> jwoolley01/09/18 23:34:11
> 
>   Modified:server/mpm/worker worker.c
>   Log:
>   I was kinda hoping those (void)some_function() and (request_rec *)NULL
>   casts would go away before this committed, but alas I didn't say anything.
>   :-)  This gets rid of them and a few others just like them that I also
>   found in worker.c.

I don't know what the groupthink is on this, or if there is a precedent
set, but I think the (void)s make it obvious to the reader that we
are deliberately throwing away the return value.

As for the (request_rec *)NULL thing, either way is fine for me.

-aaron

>   
>   Revision  ChangesPath
>   1.25  +5 -8  httpd-2.0/server/mpm/worker/worker.c
>   
>   Index: worker.c
>   ===
>   RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
>   retrieving revision 1.24
>   retrieving revision 1.25
>   diff -u -d -u -r1.24 -r1.25
>   --- worker.c2001/09/19 05:58:09 1.24
>   +++ worker.c2001/09/19 06:34:11 1.25
>   @@ -483,7 +483,7 @@
>long conn_id = AP_ID_FROM_CHILD_THREAD(my_child_num, my_thread_num);
>int csd;
>
>   -(void) apr_os_sock_get(&csd, sock);
>   +apr_os_sock_get(&csd, sock);
>
>if (csd >= FD_SETSIZE) {
>ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_WARNING, 0, NULL,
>   @@ -697,11 +697,9 @@
>
>free(ti);
>
>   -(void) ap_update_child_status(process_slot, thread_slot,
>   -  SERVER_STARTING, (request_rec *)NULL);
>   +ap_update_child_status(process_slot, thread_slot, SERVER_STARTING, NULL);
>while (!workers_may_exit) {
>   -(void) ap_update_child_status(process_slot, thread_slot,
>   -  SERVER_READY, (request_rec *)NULL);
>   +ap_update_child_status(process_slot, thread_slot, SERVER_READY, NULL);
>rv = ap_queue_pop(worker_queue, &csd, &ptrans);
>/* We get FD_QUEUE_EINTR whenever ap_queue_pop() has been interrupted
> * from an explicit call to ap_queue_interrupt_all(). This allows
>   @@ -778,8 +776,7 @@
>   my_info->sd = 0;
>   
>   /* We are creating threads right now */
>   -   (void) ap_update_child_status(my_child_num, i, SERVER_STARTING, 
>   - (request_rec *) NULL);
>   +   ap_update_child_status(my_child_num, i, SERVER_STARTING, NULL);
>/* We let each thread update its own scoreboard entry.  This is
> * done because it lets us deal with tid better.
>*/
>   @@ -947,7 +944,7 @@
>/* fork didn't succeed. Fix the scoreboard or else
> * it will say SERVER_STARTING forever and ever
> */
>   -(void) ap_update_child_status(slot, 0, SERVER_DEAD, (request_rec *) NULL);
>   +ap_update_child_status(slot, 0, SERVER_DEAD, NULL);
>
>   /* In case system resources are maxxed out, we don't want
>  Apache running away with the CPU trying to fork over and
>   
>   
>   



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-08-26 Thread dougm

On Sat, 25 Aug 2001, Ryan Bloom wrote:

> 
> This will break things.  We will no longer be keeping track of the listener, which
> is a bad thing.  I will fix this later this weekend, but the worker MPM is broken
> now.

its working for me.  was broken before, with this config:


StartServers 1
MaxClients   1
MinSpareThreads  1
MaxSpareThreads  1
ThreadsPerChild  1
MaxRequestsPerChild  0


httpd cannot serve any requests.
i don't know the code at all but ap_threads_per_child is compared to
threads_created, which is only incremented for request threads.
threads_created is not incremented when the listener threads is created.
so i don't see how the listener thread was ever "kept track of".




Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-08-26 Thread Aaron Bannert

I probably should have mentioned in the comment that the reason I made
the comment was that it broke when I changed it to 0. :)

And I assume the first question in my comment is true? That is:
"ap_threads_per_child includes the listener thread"

-aaron

On Sat, Aug 25, 2001 at 06:29:49PM -0700, Ryan Bloom wrote:
> 
> This will break things.  We will no longer be keeping track of the listener, which
> is a bad thing.  I will fix this later this weekend, but the worker MPM is broken
> now.



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-08-25 Thread Ryan Bloom

On Saturday 25 August 2001 18:40, [EMAIL PROTECTED] wrote:
> On Sat, 25 Aug 2001, Ryan Bloom wrote:
> > This will break things.  We will no longer be keeping track of the
> > listener, which is a bad thing.  I will fix this later this weekend, but
> > the worker MPM is broken now.
>
> its working for me.  was broken before, with this config:
>
> 
> StartServers 1
> MaxClients   1
> MinSpareThreads  1
> MaxSpareThreads  1
> ThreadsPerChild  1
> MaxRequestsPerChild  0
> 
>
> httpd cannot serve any requests.
> i don't know the code at all but ap_threads_per_child is compared to
> threads_created, which is only incremented for request threads.
> threads_created is not incremented when the listener threads is created.
> so i don't see how the listener thread was ever "kept track of".

I agree that it was broken before, but the shutdown and restart most likely
won't work anymore.  Someplace, we have to have the status of the listener
thread.  Before this patch, the parent process could always tell what the 
child was doing by looking at scoreboard[0].  We can't do that anymore.

I should have documented that, but I didn't.  This MPM is much better than it
used to be, but it still needs some work.  I am not suggesting backing out
this change, just wanted to make people aware that there is still work
needed on this MPM.

Ryan

__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

2001-08-25 Thread Ryan Bloom


This will break things.  We will no longer be keeping track of the listener, which
is a bad thing.  I will fix this later this weekend, but the worker MPM is broken
now.

Ryan

On Saturday 25 August 2001 18:17, [EMAIL PROTECTED] wrote:
> dougm   01/08/25 18:17:32
>
>   Modified:server/mpm/worker worker.c
>   Log:
>   i think the answer to aaron's question is "a typo".  otherwise there is
>   actually 1 less thread available to serve requests than configured.
>
>   Revision  ChangesPath
>   1.16  +2 -3  httpd-2.0/server/mpm/worker/worker.c
>
>   Index: worker.c
>   ===
>   RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
>   retrieving revision 1.15
>   retrieving revision 1.16
>   diff -u -r1.15 -r1.16
>   --- worker.c2001/08/24 16:49:39 1.15
>   +++ worker.c2001/08/26 01:17:32 1.16
>   @@ -755,9 +755,8 @@
>my_info->sd = 0;
>apr_thread_create(&listener, thread_attr, listener_thread, my_info,
> pchild); while (1) {
>   -/* Does ap_threads_per_child include the listener thread?
>   - * Why does this forloop start at 1? -aaron */
>   -for (i = 1; i < ap_threads_per_child; i++) {
>   +/* Does ap_threads_per_child include the listener thread? */
>   +for (i = 0; i < ap_threads_per_child; i++) {
>int status =
> ap_scoreboard_image->servers[child_num_arg][i].status;
>
>if (status != SERVER_GRACEFUL && status != SERVER_DEAD) {

-- 

__
Ryan Bloom  [EMAIL PROTECTED]
Covalent Technologies   [EMAIL PROTECTED]
--



<    1   2