httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Stoddard" <b...@wstoddard.com>
Subject Re: [PATCH 2] worker MPM deadlock
Date Wed, 22 May 2002 17:31:06 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

Yep this is a problem. I believe the pthread spec states that the cond_wait can be
spuriously triggered.

Bill



Mime
View raw message