httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Pane <brian.p...@cnet.com>
Subject Re: [PATCH 2] worker MPM deadlock
Date Wed, 22 May 2002 16:53:41 GMT
Bill Stoddard wrote:

>I don't know if this will actually work, haven't fully considered all failure
>possibilities. I think there are better implementations of the basic idea. Need to use
>atomic increment/decrement and replace the yield() call with pthread_yield() or
>whatever...
>
>Bill
>
>===================================================================
>RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
>retrieving revision 1.117
>diff -u -r1.117 worker.c
>--- worker.c 18 Apr 2002 17:46:20 -0000 1.117
>+++ worker.c 26 Apr 2002 17:59:16 -0000
>@@ -156,6 +156,7 @@
>  */
>
> int ap_threads_per_child = 0;         /* Worker threads per child */
>+static int worker_thread_cnt = 0;
> static int ap_daemons_to_start = 0;
> static int min_spare_threads = 0;
> static int max_spare_threads = 0;
>@@ -693,6 +694,14 @@
>         }
>         if (listener_may_exit) break;
>
>+        /* If no worker threads are available, yield our quanta and try again
>+         * later
>+         */
>+        if (!worker_thread_cnt) {
>+            yield();
>+            continue;
>+        }
>+
>         if ((rv = SAFE_ACCEPT(apr_proc_mutex_lock(accept_mutex)))
>             != APR_SUCCESS) {
>             int level = APLOG_EMERG;
>@@ -791,6 +800,7 @@
>                 signal_threads(ST_GRACEFUL);
>             }
>             if (csd != NULL) {
>+                worker_thread_cnt--;
>                 rv = ap_queue_push(worker_queue, csd, ptrans,
>                                    &recycled_pool);
>                 if (rv) {
>@@ -852,6 +862,7 @@
>
>     while (!workers_may_exit) {
>         ap_update_child_status_from_indexes(process_slot, thread_slot, SERVER_READY,
>NULL);
>+        worker_thread_cnt++;
>         rv = ap_queue_pop(worker_queue, &csd, &ptrans, last_ptrans);
>         last_ptrans = NULL;
>

Thanks for re-posting.  At first glance, what I like about this patch is
that it creates a model in which the workers produce "tokens" and the 
listener
consumes the tokens.  This removes some of the race conditions associated
with the my patch in which the workers do both the increment and the 
decrement.

However, there's a problem with this approach (aside from the whole issue of
spinning around the (worker_thread_count != 0) check).  If the 
ap_queue_pop()
function returns without actually acquiring a connection, you'll end up
doubly incrementing the idle worker count (as the worker loop enters its
next iteration).  This can happen if the cond_wait gets interrupted.  I
believe Aaron has some test cases in which this has happened, and I saw
what I think is this same phenomenon yesterday when debugging.

--Brian



Mime
View raw message