On 02/08/2011 10:08 PM, j...@apache.org wrote: > Author: jim > Date: Tue Feb 8 21:08:10 2011 > New Revision: 1068581 > > URL: http://svn.apache.org/viewvc?rev=1068581&view=rev > Log: > Remove the thread mutex from the worker... it really should be > in the balancer. Thus we have global and thread for the balancer. > Use global when updating the full, shm list of workers; use > thread when being local. > > Modified: > httpd/httpd/trunk/modules/proxy/mod_proxy.h > httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c > httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c > httpd/httpd/trunk/modules/proxy/proxy_util.c > > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1068581&r1=1068580&r2=1068581&view=diff > ============================================================================== > --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original) > +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Tue Feb 8 21:08:10 2011 > @@ -375,7 +376,6 @@ struct proxy_worker { > proxy_conn_pool *cp; /* Connection pool to use */ > proxy_worker_shared *s; /* Shared data */ > proxy_balancer *balancer; /* which balancer am I in? */ > - apr_thread_mutex_t *mutex; /* Thread lock for updating address cache */
Don't we need to protect the address cache for unbalanced workers as well by a thread lock? > void *context; /* general purpose storage */ > }; > > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=1068581&r1=1068580&r2=1068581&view=diff > ============================================================================== > --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original) > +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Tue Feb 8 21:08:10 > 2011 >> @@ -336,12 +336,7 @@ static proxy_worker *find_best_worker(pr > if (candidate) > candidate->s->elected++; Hm, why don't we need to protect the data in the shared mem like candidate->s->elected or the other elements that are touched by the finder with a global lock? > > -/* > - PROXY_GLOBAL_UNLOCK(conf); > - return NULL; > -*/ > - > - if ((rv = PROXY_GLOBAL_UNLOCK(balancer)) != APR_SUCCESS) { > + if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) { > ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server, > "proxy: BALANCER: (%s). Unlock failed for find_best_worker()", > balancer->name); > } > @@ -463,7 +458,7 @@ static int proxy_balancer_pre_request(pr > /* Step 2: Lock the LoadBalancer > * XXX: perhaps we need the process lock here > */ > - if ((rv = PROXY_GLOBAL_LOCK(*balancer)) != APR_SUCCESS) { > + if ((rv = PROXY_THREAD_LOCK(*balancer)) != APR_SUCCESS) { Same question as above: We change data in the shared mem. So why only a thread lock? > ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server, > "proxy: BALANCER: (%s). Lock failed for pre_request", > (*balancer)->name); > @@ -614,7 +609,7 @@ static int proxy_balancer_post_request(p > > apr_status_t rv; > > - if ((rv = PROXY_GLOBAL_LOCK(balancer)) != APR_SUCCESS) { > + if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) { I assume you wanted PROXY_THREAD_LOCK instead of PROXY_THREAD_UNLOCK :-) Same question as above: We change data in the shared mem. So why only a thread lock? > ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server, > "proxy: BALANCER: (%s). Lock failed for post_request", > balancer->name); > @@ -878,13 +875,13 @@ static int balancer_handler(request_rec > balancer = (proxy_balancer *)conf->balancers->elts; > for (i = 0; i < conf->balancers->nelts; i++, balancer++) { > apr_status_t rv; > - if ((rv = PROXY_GLOBAL_LOCK(balancer)) != APR_SUCCESS) { > + if ((rv = PROXY_THREAD_LOCK(balancer)) != APR_SUCCESS) { > ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server, > "proxy: BALANCER: (%s). Lock failed for > balancer_handler", > balancer->name); > } > ap_proxy_update_members(balancer, r->server, conf); Can't the data change in shared memory while we update our local data, e.g. by balancer_handler that is executed in another process and that adds or removes members at the same time? > - if ((rv = PROXY_GLOBAL_UNLOCK(balancer)) != APR_SUCCESS) { > + if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) { > ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server, > "proxy: BALANCER: (%s). Unlock failed for > balancer_handler", > balancer->name); Regards Rüdiger
