You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by aa...@apache.org on 2002/04/28 03:45:00 UTC
cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
aaron 02/04/27 18:45:00
Modified: server/mpm/worker worker.c fdqueue.c fdqueue.h
Log:
Add a "queue_info" structure to the worker MPM. This is used to prevent
the listener thread from accept()ing more connections than there are
available workers. This prevents long-running requests from starving
connections that have been accepted but not yet processed.
The queue_info is a simple counter, mutex, and condition variable. Only
the listener thread blocks on the condition, and only when there are no
idle workers. In the fast path there is a mutex lock, integer decrement,
and and unlock (among a few conditionals). The worker threads each notify
the queue_info when they are about to block on the normal worker_queue
waiting for some connection to process, which wakes up any sleeping
listener thread to go perform another accept() in parallel.
Revision Changes Path
1.118 +32 -0 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.117
retrieving revision 1.118
diff -u -r1.117 -r1.118
--- worker.c 18 Apr 2002 17:46:20 -0000 1.117
+++ worker.c 28 Apr 2002 01:45:00 -0000 1.118
@@ -173,6 +173,7 @@
static int num_listensocks = 0;
static int resource_shortage = 0;
static fd_queue_t *worker_queue;
+static fd_queue_info_t *worker_queue_info;
/* The structure used to pass unique initialization info to each thread */
typedef struct {
@@ -299,6 +300,7 @@
if (mode == ST_UNGRACEFUL) {
workers_may_exit = 1;
ap_queue_interrupt_all(worker_queue);
+ ap_queue_info_term(worker_queue_info);
}
}
@@ -693,6 +695,20 @@
}
if (listener_may_exit) break;
+ rv = ap_queue_info_wait_for_idler(worker_queue_info);
+ if (APR_STATUS_IS_EOF(rv)) {
+ break; /* we've been signaled to die now */
+ }
+ else if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
+ "apr_queue_info_wait failed. Attempting to shutdown "
+ "process gracefully.");
+ signal_threads(ST_GRACEFUL);
+ break;
+ }
+ /* We've already decremented the idle worker count inside
+ * ap_queue_info_wait_for_idler. */
+
if ((rv = SAFE_ACCEPT(apr_proc_mutex_lock(accept_mutex)))
!= APR_SUCCESS) {
int level = APLOG_EMERG;
@@ -851,6 +867,15 @@
bucket_alloc = apr_bucket_alloc_create(apr_thread_pool_get(thd));
while (!workers_may_exit) {
+ rv = ap_queue_info_set_idle(worker_queue_info);
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
+ "ap_queue_info_set_idle failed. Attempting to "
+ "shutdown process gracefully.");
+ signal_threads(ST_GRACEFUL);
+ break;
+ }
+
ap_update_child_status_from_indexes(process_slot, thread_slot, SERVER_READY, NULL);
rv = ap_queue_pop(worker_queue, &csd, &ptrans, last_ptrans);
last_ptrans = NULL;
@@ -958,6 +983,13 @@
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
"ap_queue_init() failed");
+ clean_child_exit(APEXIT_CHILDFATAL);
+ }
+
+ rv = ap_queue_info_create(&worker_queue_info, pchild);
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
+ "ap_queue_info_create() failed");
clean_child_exit(APEXIT_CHILDFATAL);
}
1.16 +108 -0 httpd-2.0/server/mpm/worker/fdqueue.c
Index: fdqueue.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/worker/fdqueue.c,v
retrieving revision 1.15
retrieving revision 1.16
diff -u -r1.15 -r1.16
--- fdqueue.c 26 Apr 2002 17:13:51 -0000 1.15
+++ fdqueue.c 28 Apr 2002 01:45:00 -0000 1.16
@@ -58,6 +58,114 @@
#include "fdqueue.h"
+struct fd_queue_info_t {
+ int idlers;
+ apr_thread_mutex_t *idlers_mutex;
+ apr_thread_cond_t *wait_for_idler;
+ int terminated;
+};
+
+static apr_status_t queue_info_cleanup(void *data_)
+{
+ fd_queue_info_t *qi = data_;
+ apr_thread_cond_destroy(qi->wait_for_idler);
+ apr_thread_mutex_destroy(qi->idlers_mutex);
+ return APR_SUCCESS;
+}
+
+apr_status_t ap_queue_info_create(fd_queue_info_t **queue_info,
+ apr_pool_t *pool)
+{
+ apr_status_t rv;
+ fd_queue_info_t *qi;
+
+ qi = apr_palloc(pool, sizeof(*qi));
+ memset(qi, 0, sizeof(*qi));
+
+ rv = apr_thread_mutex_create(&qi->idlers_mutex, APR_THREAD_MUTEX_DEFAULT,
+ pool);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ rv = apr_thread_cond_create(&qi->wait_for_idler, pool);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ apr_pool_cleanup_register(pool, qi, queue_info_cleanup,
+ apr_pool_cleanup_null);
+
+ *queue_info = qi;
+
+ return APR_SUCCESS;
+}
+
+apr_status_t ap_queue_info_set_idle(fd_queue_info_t *queue_info)
+{
+ apr_status_t rv;
+ rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ AP_DEBUG_ASSERT(queue_info->idlers >= 0);
+ if (queue_info->idlers++ == 0) {
+ /* Only signal if we had no idlers before. */
+ apr_thread_cond_signal(queue_info->wait_for_idler);
+ }
+ rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ return APR_SUCCESS;
+}
+
+apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t *queue_info)
+{
+ apr_status_t rv;
+ rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ AP_DEBUG_ASSERT(queue_info->idlers >= 0);
+ while ((queue_info->idlers == 0) && (!queue_info->terminated)) {
+ rv = apr_thread_cond_wait(queue_info->wait_for_idler,
+ queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ return rv;
+ }
+ }
+ queue_info->idlers--; /* Oh, and idler? Let's take 'em! */
+ rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ else if (queue_info->terminated) {
+ return APR_EOF;
+ }
+ else {
+ return APR_SUCCESS;
+ }
+}
+
+apr_status_t ap_queue_info_term(fd_queue_info_t *queue_info)
+{
+ apr_status_t rv;
+ rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ queue_info->terminated = 1;
+ apr_thread_cond_broadcast(queue_info->wait_for_idler);
+ rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ return APR_SUCCESS;
+}
+
/**
* Detects when the fd_queue_t is full. This utility function is expected
* to be called from within critical sections, and is not threadsafe.
1.17 +8 -0 httpd-2.0/server/mpm/worker/fdqueue.h
Index: fdqueue.h
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/worker/fdqueue.h,v
retrieving revision 1.16
retrieving revision 1.17
diff -u -r1.16 -r1.17
--- fdqueue.h 26 Apr 2002 17:13:51 -0000 1.16
+++ fdqueue.h 28 Apr 2002 01:45:00 -0000 1.17
@@ -71,6 +71,14 @@
#endif
#include <apr_errno.h>
+typedef struct fd_queue_info_t fd_queue_info_t;
+
+apr_status_t ap_queue_info_create(fd_queue_info_t **queue_info,
+ apr_pool_t *pool);
+apr_status_t ap_queue_info_set_idle(fd_queue_info_t *queue_info);
+apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t *queue_info);
+apr_status_t ap_queue_info_term(fd_queue_info_t *queue_info);
+
struct fd_queue_elem_t {
apr_socket_t *sd;
apr_pool_t *p;
Re: More worker ideas Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
Posted by Aaron Bannert <aa...@clove.org>.
On Sat, Apr 27, 2002 at 07:39:24PM -0700, Brian Pane wrote:
> I was going to complain about the addition of yet another mutex
> to the critical path of request processing, but it got me thinking
> about an approach to making worker faster: shorten the time spent
> in the mutex-protected region.
Yes! The other neat thing about this is we get to balance the
critical path between two mutex regions, rather than one_big_mutex.
> By shortening the code path between the lock and the unlock, we
> can reduce the probability that two or more threads will be
> contending for the mutex at once. And uncontended mutexes have
> a lot less overhead.
>
> I'm going to experiment with streamlining the code within the
> mutex-protected blocks of the fdqueue functions to see how much
> it helps the multiprocessor performance. At first glance, the
> candidates for optimization are:
> * In the case where we can't recycle a pool in ap_queue_pop(),
> move the destruction of the pool *outside* the mutex-protected
> block.
> * Maybe change the queue size to a power of two so that we can
> replace the modulo operations with a bit mask.
++1 on both accounts. I was also thinking that we could make the queue
elements simple pointers, rather than 2 elements in a structure. That
way the element lookups can happen outside of the mutex region, and
inside we can have a simple struct* assignment.
-aaron
More worker ideas Re: cvs commit: httpd-2.0/server/mpm/worker
worker.c fdqueue.c fdqueue.h
Posted by Brian Pane <bp...@pacbell.net>.
aaron@apache.org wrote:
>aaron 02/04/27 18:45:00
>
> Modified: server/mpm/worker worker.c fdqueue.c fdqueue.h
> Log:
> Add a "queue_info" structure to the worker MPM. This is used to prevent
> the listener thread from accept()ing more connections than there are
> available workers. This prevents long-running requests from starving
> connections that have been accepted but not yet processed.
>
> The queue_info is a simple counter, mutex, and condition variable. Only
>
I was going to complain about the addition of yet another mutex
to the critical path of request processing, but it got me thinking
about an approach to making worker faster: shorten the time spent
in the mutex-protected region.
By shortening the code path between the lock and the unlock, we
can reduce the probability that two or more threads will be
contending for the mutex at once. And uncontended mutexes have
a lot less overhead.
I'm going to experiment with streamlining the code within the
mutex-protected blocks of the fdqueue functions to see how much
it helps the multiprocessor performance. At first glance, the
candidates for optimization are:
* In the case where we can't recycle a pool in ap_queue_pop(),
move the destruction of the pool *outside* the mutex-protected
block.
* Maybe change the queue size to a power of two so that we can
replace the modulo operations with a bit mask.
--Brian
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c
fdqueue.h
Posted by Brian Pane <bp...@pacbell.net>.
Bill Stoddard wrote:
>
>>On Sat, Apr 27, 2002 at 07:30:51PM -0700, Justin Erenkrantz wrote:
>>
>>>> + qi = apr_palloc(pool, sizeof(*qi));
>>>> + memset(qi, 0, sizeof(*qi));
>>>>
>>>As we said, if you are concerned about the performance aspect
>>>of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
>>>work around its inefficiencies by pointedly not using it.
>>>
>>>If/when Cliff (or someone else?) commits the change to apr_pcalloc,
>>>this chunk would be magically changed along with everything else if
>>>you simply called apr_pcalloc in the first place.
>>>
>>We don't have a consensus on this, and I'm ambivalent about making the
>>p_calloc macro. If we do come up with a consensus than it can change.
>>Until then this is more correct than using p_calloc.
>>
>
>I have no problem with using apr_palloc()/memset() in place of apr_pcalloc().
>
I'm +1 on turning pcalloc into a macro,
-0 for using pcalloc in ap_queue_info_init(),
-0.5 (non-veto) for using palloc/memset in ap_queue_info_init()
Rationale:
* Changing pcalloc to a macro will give us a free optimization
throughout the entire code base.
* In ap_queue_info_init(), we create a struct with 7 fields and
immediately set 4 of them to nonzero values. Using pcalloc, or
any other form of block zero-fill, isn't a big win.
* The whole point of the palloc/memset idiom is that
it's a more awkward syntax than pcalloc, but justified
for the sake of performance optimization. But there's
no point in optimizing ap_queue_info_init(). The function
gets called once per child proc, at startup, and no matter
what technique you use to zero-fill the newly created struct,
it's going to account for far too little of the total run
time to matter at all. There's plenty of code within
fdqueue.c that could benefit from performance optimization,
but none of it is in this function.
>>>> + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
>>>> + if (rv != APR_SUCCESS) {
>>>> + return rv;
>>>> + }
>>>> + return APR_SUCCESS;
>>>> +}
>>>>
>>>As I said before, simply "return rv;" works here.
>>>
>>Yeah, but this is much more readable.
>>
>
>I disagree. I would rather see "return rv".
>
I'd rather see "return rv" or "return apr_thread_mutex_unlock".
Both are more readable (and faster) than the current code with
the if-statement.
--Brian
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c
fdqueue.h
Posted by Cliff Woolley <jw...@virginia.edu>.
On Sun, 28 Apr 2002, Bill Stoddard wrote:
> I have no problem with using apr_palloc()/memset() in place of apr_pcalloc().
Do you have a problem with making apr_pcalloc() a macro?
> > > > + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > > > + if (rv != APR_SUCCESS) {
> > > > + return rv;
> > > > + }
> > > > + return APR_SUCCESS;
> > > > +}
> > >
> > > As I said before, simply "return rv;" works here.
> > Yeah, but this is much more readable.
> I disagree. I would rather see "return rv".
FWIW, I also prefer return rv. Or even return apr_thread_mutex_unlock();.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
Posted by Bill Stoddard <bi...@wstoddard.com>.
> On Sat, Apr 27, 2002 at 07:30:51PM -0700, Justin Erenkrantz wrote:
> > > + qi = apr_palloc(pool, sizeof(*qi));
> > > + memset(qi, 0, sizeof(*qi));
> >
> > As we said, if you are concerned about the performance aspect
> > of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
> > work around its inefficiencies by pointedly not using it.
> >
> > If/when Cliff (or someone else?) commits the change to apr_pcalloc,
> > this chunk would be magically changed along with everything else if
> > you simply called apr_pcalloc in the first place.
>
> We don't have a consensus on this, and I'm ambivalent about making the
> p_calloc macro. If we do come up with a consensus than it can change.
> Until then this is more correct than using p_calloc.
I have no problem with using apr_palloc()/memset() in place of apr_pcalloc().
>
> > > + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > > + if (rv != APR_SUCCESS) {
> > > + return rv;
> > > + }
> > > + return APR_SUCCESS;
> > > +}
> >
> > As I said before, simply "return rv;" works here.
>
> Yeah, but this is much more readable.
I disagree. I would rather see "return rv".
My .02.
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
Posted by Aaron Bannert <aa...@clove.org>.
On Sat, Apr 27, 2002 at 07:30:51PM -0700, Justin Erenkrantz wrote:
> > + qi = apr_palloc(pool, sizeof(*qi));
> > + memset(qi, 0, sizeof(*qi));
>
> As we said, if you are concerned about the performance aspect
> of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
> work around its inefficiencies by pointedly not using it.
>
> If/when Cliff (or someone else?) commits the change to apr_pcalloc,
> this chunk would be magically changed along with everything else if
> you simply called apr_pcalloc in the first place.
We don't have a consensus on this, and I'm ambivalent about making the
p_calloc macro. If we do come up with a consensus than it can change.
Until then this is more correct than using p_calloc.
> > + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > + if (rv != APR_SUCCESS) {
> > + return rv;
> > + }
> > + return APR_SUCCESS;
> > +}
>
> As I said before, simply "return rv;" works here.
Yeah, but this is much more readable.
> > + if (rv != APR_SUCCESS) {
> > + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> > + if (rv != APR_SUCCESS) {
> > + return rv;
> > + }
> > + return rv;
> > + }
>
> Ditto.
This is legit, I'll fix it.
-aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c fdqueue.h
Posted by Justin Erenkrantz <je...@apache.org>.
Comments inline.
On Sun, Apr 28, 2002 at 01:45:00AM -0000, aaron@apache.org wrote:
> 1.16 +108 -0 httpd-2.0/server/mpm/worker/fdqueue.c
>
> Index: fdqueue.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/server/mpm/worker/fdqueue.c,v
> retrieving revision 1.15
> retrieving revision 1.16
> diff -u -r1.15 -r1.16
> --- fdqueue.c 26 Apr 2002 17:13:51 -0000 1.15
> +++ fdqueue.c 28 Apr 2002 01:45:00 -0000 1.16
> @@ -58,6 +58,114 @@
>
> #include "fdqueue.h"
>
> +struct fd_queue_info_t {
> + int idlers;
> + apr_thread_mutex_t *idlers_mutex;
> + apr_thread_cond_t *wait_for_idler;
> + int terminated;
> +};
> +
> +static apr_status_t queue_info_cleanup(void *data_)
> +{
> + fd_queue_info_t *qi = data_;
> + apr_thread_cond_destroy(qi->wait_for_idler);
> + apr_thread_mutex_destroy(qi->idlers_mutex);
> + return APR_SUCCESS;
> +}
> +
> +apr_status_t ap_queue_info_create(fd_queue_info_t **queue_info,
> + apr_pool_t *pool)
> +{
> + apr_status_t rv;
> + fd_queue_info_t *qi;
> +
> + qi = apr_palloc(pool, sizeof(*qi));
> + memset(qi, 0, sizeof(*qi));
As we said, if you are concerned about the performance aspect
of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
work around its inefficiencies by pointedly not using it.
If/when Cliff (or someone else?) commits the change to apr_pcalloc,
this chunk would be magically changed along with everything else if
you simply called apr_pcalloc in the first place.
> +
> + rv = apr_thread_mutex_create(&qi->idlers_mutex, APR_THREAD_MUTEX_DEFAULT,
> + pool);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + rv = apr_thread_cond_create(&qi->wait_for_idler, pool);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + apr_pool_cleanup_register(pool, qi, queue_info_cleanup,
> + apr_pool_cleanup_null);
> +
> + *queue_info = qi;
> +
> + return APR_SUCCESS;
> +}
> +
> +apr_status_t ap_queue_info_set_idle(fd_queue_info_t *queue_info)
> +{
> + apr_status_t rv;
> + rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + AP_DEBUG_ASSERT(queue_info->idlers >= 0);
> + if (queue_info->idlers++ == 0) {
> + /* Only signal if we had no idlers before. */
> + apr_thread_cond_signal(queue_info->wait_for_idler);
> + }
> + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + return APR_SUCCESS;
> +}
As I said before, simply "return rv;" works here.
> +apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t *queue_info)
> +{
> + apr_status_t rv;
> + rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + AP_DEBUG_ASSERT(queue_info->idlers >= 0);
> + while ((queue_info->idlers == 0) && (!queue_info->terminated)) {
> + rv = apr_thread_cond_wait(queue_info->wait_for_idler,
> + queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + return rv;
> + }
Ditto.
> + }
> + queue_info->idlers--; /* Oh, and idler? Let's take 'em! */
> + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + else if (queue_info->terminated) {
> + return APR_EOF;
> + }
> + else {
> + return APR_SUCCESS;
> + }
> +}
> +
> +apr_status_t ap_queue_info_term(fd_queue_info_t *queue_info)
> +{
> + apr_status_t rv;
> + rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + queue_info->terminated = 1;
> + apr_thread_cond_broadcast(queue_info->wait_for_idler);
> + rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + return APR_SUCCESS;
> +}
Ditto. -- justin