You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Paul J. Reder" <re...@raleigh.ibm.com> on 2001/04/14 21:31:02 UTC
[Patch]: Patch to make threaded mpm use join to cleanup workers.
This is the patch that I will commit if I can't find a valid reason not to.
This is an FYI heads up only. I am currently looking for any history
associated with using join in past Apache code. If I cannot find any
then I will commit this later today.
I tested this under extremely heavy load (almost 10 million requests over
a 10 hour period passing over 80 GB of data). And the server didn't have
a single problem until the machine ran out of memory (my config fault).
Actually it had run out of memory twice earlier on in the process but
recovered and continued. It finally gave up when one of the workers
returned a fatal error. But at the time the fatal error occured, there
wasn't a single orphaned thread.
The only reason performance wasn't better was related to one of the other
problems that I have pointed out (a child_main server process cannot be
replaced until it exits - even if it is left with only one or two threads
finishing their last (long) request). I will address this performance issue
in another patch.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Index: httpd-2.0/server/mpm/threaded/threaded.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/threaded/threaded.c,v
retrieving revision 1.21
diff -u -r1.21 threaded.c
--- httpd-2.0/server/mpm/threaded/threaded.c 2001/04/12 18:46:32 1.21
+++ httpd-2.0/server/mpm/threaded/threaded.c 2001/04/14 17:51:31
@@ -544,8 +544,7 @@
if (!workers_may_exit) {
if ((rv = apr_accept(&csd, sd, ptrans)) != APR_SUCCESS) {
csd = NULL;
- ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf,
- "apr_accept");
+ ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf, "apr_accept");
}
if ((rv = SAFE_ACCEPT(apr_lock_release(accept_mutex)))
!= APR_SUCCESS) {
@@ -573,8 +572,7 @@
}
apr_pool_destroy(tpool);
- ap_update_child_status(process_slot, thread_slot, SERVER_DEAD,
- (request_rec *) NULL);
+ ap_update_child_status(process_slot, thread_slot, SERVER_DEAD, (request_rec *) NULL);
apr_lock_acquire(worker_thread_count_mutex);
worker_thread_count--;
if (worker_thread_count == 0) {
@@ -592,15 +590,14 @@
switch (signum) {
case SIGTERM:
case SIGINT:
- just_die(signum);
return 1;
- }
+ }
return 0;
}
static void child_main(int child_num_arg)
{
- apr_thread_t *thread;
+ apr_thread_t **threads;
apr_threadattr_t *thread_attr;
int i;
int my_child_num = child_num_arg;
@@ -650,13 +647,20 @@
listensocks[i]=lr->sd;
/* Setup worker threads */
+
+ threads = (apr_thread_t **)malloc(sizeof(apr_thread_t *) * ap_threads_per_child);
+ if (threads == NULL) {
+ ap_log_error(APLOG_MARK, APLOG_ALERT, errno, ap_server_conf,
+ "malloc: out of memory");
+ clean_child_exit(APEXIT_CHILDFATAL);
+ }
worker_thread_count = 0;
apr_lock_create(&worker_thread_count_mutex, APR_MUTEX, APR_INTRAPROCESS,
NULL, pchild);
apr_lock_create(&pipe_of_death_mutex, APR_MUTEX, APR_INTRAPROCESS,
NULL, pchild);
apr_threadattr_create(&thread_attr, pchild);
- apr_threadattr_detach_set(thread_attr, 1);
+ apr_threadattr_detach_set(thread_attr, PTHREAD_CREATE_JOINABLE);
for (i=0; i < ap_threads_per_child; i++) {
@@ -674,7 +678,7 @@
/* We are creating threads right now */
(void) ap_update_child_status(my_child_num, i, SERVER_STARTING,
(request_rec *) NULL);
- if ((rv = apr_thread_create(&thread, thread_attr, worker_thread, my_info, pchild))) {
+ if ((rv = apr_thread_create(&threads[i], thread_attr, worker_thread, my_info, pchild))) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
"apr_thread_create: unable to create worker thread");
/* In case system resources are maxxed out, we don't want
@@ -687,7 +691,23 @@
* because it let's us deal with tid better.
*/
}
+
+ /* What state should this child_main process be listed as in the scoreboard...?
+ ap_update_child_status(my_child_num, i, SERVER_STARTING, (request_rec *) NULL);
+ */
+
apr_signal_thread(check_signal);
+
+ /* A terminating signal was received. Now join each of the workers to clean them up. */
+ /* If the worker already exitted, then the join frees their resources and returns. */
+ /* If the worker hasn't exited, then this blocks until they have (then cleans up). */
+ for (i = 0; i < ap_threads_per_child; i++) {
+ apr_thread_join(&rv, threads[i]);
+ }
+
+ free (threads);
+
+ clean_child_exit(0);
}
static int make_child(server_rec *s, int slot)
Re: [Patch]: Patch to make threaded mpm use join to cleanup workers.
Posted by Bill Stoddard <bi...@wstoddard.com>.
+1
----- Original Message -----
From: "Paul J. Reder" <re...@raleigh.ibm.com>
To: <ne...@apache.org>
Sent: Saturday, April 14, 2001 3:31 PM
Subject: [Patch]: Patch to make threaded mpm use join to cleanup workers.
> This is the patch that I will commit if I can't find a valid reason not to.
> This is an FYI heads up only. I am currently looking for any history
> associated with using join in past Apache code. If I cannot find any
> then I will commit this later today.
>
> I tested this under extremely heavy load (almost 10 million requests over
> a 10 hour period passing over 80 GB of data). And the server didn't have
> a single problem until the machine ran out of memory (my config fault).
> Actually it had run out of memory twice earlier on in the process but
> recovered and continued. It finally gave up when one of the workers
> returned a fatal error. But at the time the fatal error occured, there
> wasn't a single orphaned thread.
>
> The only reason performance wasn't better was related to one of the other
> problems that I have pointed out (a child_main server process cannot be
> replaced until it exits - even if it is left with only one or two threads
> finishing their last (long) request). I will address this performance issue
> in another patch.
>
> --
> Paul J. Reder
> -----------------------------------------------------------
> "The strength of the Constitution lies entirely in the determination of each
> citizen to defend it. Only if every single citizen feels duty bound to do
> his share in this defense are the constitutional rights secure."
> -- Albert Einstein
>
>
>
> Index: httpd-2.0/server/mpm/threaded/threaded.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/server/mpm/threaded/threaded.c,v
> retrieving revision 1.21
> diff -u -r1.21 threaded.c
> --- httpd-2.0/server/mpm/threaded/threaded.c 2001/04/12 18:46:32 1.21
> +++ httpd-2.0/server/mpm/threaded/threaded.c 2001/04/14 17:51:31
> @@ -544,8 +544,7 @@
> if (!workers_may_exit) {
> if ((rv = apr_accept(&csd, sd, ptrans)) != APR_SUCCESS) {
> csd = NULL;
> - ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf,
> - "apr_accept");
> + ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf, "apr_accept");
> }
> if ((rv = SAFE_ACCEPT(apr_lock_release(accept_mutex)))
> != APR_SUCCESS) {
> @@ -573,8 +572,7 @@
> }
>
> apr_pool_destroy(tpool);
> - ap_update_child_status(process_slot, thread_slot, SERVER_DEAD,
> - (request_rec *) NULL);
> + ap_update_child_status(process_slot, thread_slot, SERVER_DEAD, (request_rec *)
NULL);
> apr_lock_acquire(worker_thread_count_mutex);
> worker_thread_count--;
> if (worker_thread_count == 0) {
> @@ -592,15 +590,14 @@
> switch (signum) {
> case SIGTERM:
> case SIGINT:
> - just_die(signum);
> return 1;
> - }
> + }
> return 0;
> }
>
> static void child_main(int child_num_arg)
> {
> - apr_thread_t *thread;
> + apr_thread_t **threads;
> apr_threadattr_t *thread_attr;
> int i;
> int my_child_num = child_num_arg;
> @@ -650,13 +647,20 @@
> listensocks[i]=lr->sd;
>
> /* Setup worker threads */
> +
> + threads = (apr_thread_t **)malloc(sizeof(apr_thread_t *) * ap_threads_per_child);
> + if (threads == NULL) {
> + ap_log_error(APLOG_MARK, APLOG_ALERT, errno, ap_server_conf,
> + "malloc: out of memory");
> + clean_child_exit(APEXIT_CHILDFATAL);
> + }
> worker_thread_count = 0;
> apr_lock_create(&worker_thread_count_mutex, APR_MUTEX, APR_INTRAPROCESS,
> NULL, pchild);
> apr_lock_create(&pipe_of_death_mutex, APR_MUTEX, APR_INTRAPROCESS,
> NULL, pchild);
> apr_threadattr_create(&thread_attr, pchild);
> - apr_threadattr_detach_set(thread_attr, 1);
> + apr_threadattr_detach_set(thread_attr, PTHREAD_CREATE_JOINABLE);
>
> for (i=0; i < ap_threads_per_child; i++) {
>
> @@ -674,7 +678,7 @@
> /* We are creating threads right now */
> (void) ap_update_child_status(my_child_num, i, SERVER_STARTING,
> (request_rec *) NULL);
> - if ((rv = apr_thread_create(&thread, thread_attr, worker_thread, my_info, pchild))) {
> + if ((rv = apr_thread_create(&threads[i], thread_attr, worker_thread, my_info,
pchild))) {
> ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
> "apr_thread_create: unable to create worker thread");
> /* In case system resources are maxxed out, we don't want
> @@ -687,7 +691,23 @@
> * because it let's us deal with tid better.
> */
> }
> +
> + /* What state should this child_main process be listed as in the scoreboard...?
> + ap_update_child_status(my_child_num, i, SERVER_STARTING, (request_rec *) NULL);
> + */
> +
> apr_signal_thread(check_signal);
> +
> + /* A terminating signal was received. Now join each of the workers to clean them
up. */
> + /* If the worker already exitted, then the join frees their resources and
returns. */
> + /* If the worker hasn't exited, then this blocks until they have (then cleans
up). */
> + for (i = 0; i < ap_threads_per_child; i++) {
> + apr_thread_join(&rv, threads[i]);
> + }
> +
> + free (threads);
> +
> + clean_child_exit(0);
> }
>
> static int make_child(server_rec *s, int slot)
>
Re: [Patch]: Patch to make threaded mpm use join to cleanup workers.
Posted by rb...@covalent.net.
On Sat, 14 Apr 2001, Paul J. Reder wrote:
> rbb@covalent.net wrote:
> > Please remove any formating issues from the patch. They just clutter what
> > you are really doing here.
>
> Yeah, sorry. Missed those in the lookover. I had played with some code and
> put it back wrong.
>
> >
> > > - apr_threadattr_detach_set(thread_attr, 1);
> > > + apr_threadattr_detach_set(thread_attr, PTHREAD_CREATE_JOINABLE);
> >
> > This is a big no-no. You can't use a PTHREAD macro in an apr_thread call.
>
> Um, ok. Then I should just assume that I can guess that passing 0 will do
> the right thing? Or is there an APR equivalent to this that I missed?
> The APR code acts like it just wants this parm to be 0/1 (i.e. off or on),
> but passes the value straight through to the pthread call (which expects the
> enum).
I believe passing in zero just does the right thing. I haven't reviewed
this code for a long time, but if you look at testthread.c, we don't set
it at all, and all of the threads are joined at the end of the program.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: Patch to make threaded mpm use join to cleanup workers.
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
rbb@covalent.net wrote:
> Please remove any formating issues from the patch. They just clutter what
> you are really doing here.
Yeah, sorry. Missed those in the lookover. I had played with some code and
put it back wrong.
>
> > - apr_threadattr_detach_set(thread_attr, 1);
> > + apr_threadattr_detach_set(thread_attr, PTHREAD_CREATE_JOINABLE);
>
> This is a big no-no. You can't use a PTHREAD macro in an apr_thread call.
Um, ok. Then I should just assume that I can guess that passing 0 will do
the right thing? Or is there an APR equivalent to this that I missed?
The APR code acts like it just wants this parm to be 0/1 (i.e. off or on),
but passes the value straight through to the pthread call (which expects the
enum).
Thanks,
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [Patch]: Patch to make threaded mpm use join to cleanup workers.
Posted by rb...@covalent.net.
All in all the patch looks good, just a few comments. BTW, the reason the
join was removed a few years ago, is that we were doing it in
clean_child_exit, which meant it was happening during signal handling,
which is bad.
> if ((rv = apr_accept(&csd, sd, ptrans)) != APR_SUCCESS) {
> csd = NULL;
> - ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf,
> - "apr_accept");
> + ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf, "apr_accept");
Please remove any formating issues from the patch. They just clutter what
you are really doing here.
> - apr_threadattr_detach_set(thread_attr, 1);
> + apr_threadattr_detach_set(thread_attr, PTHREAD_CREATE_JOINABLE);
This is a big no-no. You can't use a PTHREAD macro in an apr_thread call.
> +
> + /* What state should this child_main process be listed as in the scoreboard...?
> + ap_update_child_status(my_child_num, i, SERVER_STARTING, (request_rec *) NULL);
> + */
Please fix these comments so that they don't wrap, and use the Apache
comment style.
> apr_signal_thread(check_signal);
> +
> + /* A terminating signal was received. Now join each of the workers to clean them up. */
> + /* If the worker already exitted, then the join frees their resources and returns. */
> + /* If the worker hasn't exited, then this blocks until they have (then cleans up). */
Same as above.
Other than those few comments, please commit.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------