On 9/11/23 3:50 PM, jfcl...@apache.org wrote:
> Author: jfclere
> Date: Mon Sep 11 13:50:21 2023
> New Revision: 1912245
>
> URL: http://svn.apache.org/viewvc?rev=1912245&view=rev
> Log:
> Arrange the bybusyness logic and prevent bad busy values
> this closes #383
>
> Modified:
> httpd/httpd/trunk/modules/proxy/balancers/mod_lbmethod_bybusyness.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
> httpd/httpd/trunk/modules/proxy/proxy_util.h
>
> 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=1912245&r1=1912244&r2=1912245&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Mon Sep 11 13:50:21
> 2023
> @@ -17,6 +17,7 @@
> /* Load balancer module for Apache proxy */
>
> #include "mod_proxy.h"
> +#include "proxy_util.h"
> #include "scoreboard.h"
> #include "ap_mpm.h"
> #include "apr_version.h"
> @@ -486,17 +487,6 @@ static void force_recovery(proxy_balance
> }
> }
>
> -static apr_status_t decrement_busy_count(void *worker_)
> -{
> - proxy_worker *worker = worker_;
> -
> - if (worker->s->busy) {
> - worker->s->busy--;
> - }
> -
> - return APR_SUCCESS;
> -}
> -
> static int proxy_balancer_pre_request(proxy_worker **worker,
> proxy_balancer **balancer,
> request_rec *r,
> @@ -635,7 +625,7 @@ static int proxy_balancer_pre_request(pr
> *worker = runtime;
> }
>
> - (*worker)->s->busy++;
> + increment_busy_count(*worker);
> apr_pool_cleanup_register(r->pool, *worker, decrement_busy_count,
> apr_pool_cleanup_null);
>
> @@ -1575,7 +1565,7 @@ static void balancer_display_page(reques
> "</httpd:redirect>\n", NULL);
> ap_rprintf(r,
> " <httpd:busy>%" APR_SIZE_T_FMT
> "</httpd:busy>\n",
> - worker->s->busy);
> + getbusy_count(worker));
> ap_rprintf(r, " <httpd:lbset>%d</httpd:lbset>\n",
> worker->s->lbset);
> /* End proxy_worker_stat */
> @@ -1748,7 +1738,7 @@ static void balancer_display_page(reques
> ap_rvputs(r, ap_proxy_parse_wstatus(r->pool, worker), NULL);
> ap_rputs("</td>", r);
> ap_rprintf(r, "<td>%" APR_SIZE_T_FMT "</td>",
> worker->s->elected);
> - ap_rprintf(r, "<td>%" APR_SIZE_T_FMT "</td>",
> worker->s->busy);
> + ap_rprintf(r, "<td>%" APR_SIZE_T_FMT "</td>",
> getbusy_count(worker));
> ap_rprintf(r, "<td>%d</td><td>", worker->s->lbstatus);
> ap_rputs(apr_strfsize(worker->s->transferred, fbuf), r);
> ap_rputs("</td><td>", r);
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1912245&r1=1912244&r2=1912245&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Sep 11 13:50:21 2023
> @@ -21,6 +21,7 @@
> #include "apr_version.h"
> #include "apr_strings.h"
> #include "apr_hash.h"
> +#include "apr_atomic.h"
> #include "http_core.h"
> #include "proxy_util.h"
> #include "ajp.h"
> @@ -4984,6 +4985,124 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tun
> return APR_SUCCESS;
> }
>
> +PROXY_DECLARE(apr_status_t) decrement_busy_count(void *worker_)
Why can't we use the respective apr_atomic_sub/add/inc/dec* functions here
instead of the logic below?
> +{
> + apr_size_t val;
> + proxy_worker *worker = worker_;
> +
> +#if APR_SIZEOF_VOIDP == 4
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> + val = apr_atomic_read32(&worker->s->busy);
> + while (val > 0) {
> + apr_size_t old = val;
> + val = apr_atomic_cas32(&worker->s->busy, val - 1, old);
> + if (val == old) {
> + break;
> + }
> + }
> +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before 1.7.4
> */
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> + val = apr_atomic_read64(&worker->s->busy);
> + while (val > 0) {
> + apr_size_t old = val;
> + val = apr_atomic_cas64(&worker->s->busy, val - 1, old);
> + if (val == old) {
> + break;
> + }
> + }
> +#else /* Use atomics for (64bit) pointers */
> + void *volatile *busy_p = (void *)&worker->s->busy;
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> + AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> + val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> + while (val > 0) {
> + apr_size_t old = val;
> + val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p,
> + (void *)(apr_uintptr_t)(val -
> 1),
> + (void *)(apr_uintptr_t)old);
> + if (val == old) {
> + break;
> + }
> + }
> +#endif
> + return APR_SUCCESS;
> +}
> +
> +PROXY_DECLARE(void) increment_busy_count(proxy_worker *worker)
> +{
> + apr_size_t val;
> +#if APR_SIZEOF_VOIDP == 4
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> + val = apr_atomic_read32(&worker->s->busy);
> + while (val < APR_INT32_MAX) {
> + apr_size_t old = val;
> + val = apr_atomic_cas32(&worker->s->busy, val + 1, old);
> + if (val == old) {
> + break;
> + }
> + }
> +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before 1.7.4
> */
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> + val = apr_atomic_read64(&worker->s->busy);
> + while (val < APR_INT64_MAX) {
> + apr_size_t old = val;
> + val = apr_atomic_cas64(&worker->s->busy, val + 1, old);
> + if (val == old) {
> + break;
> + }
> + }
> +#else /* Use atomics for (64bit) pointers */
> + void *volatile *busy_p = (void *)&worker->s->busy;
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> + AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> + val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> + while (val < APR_INT64_MAX) {
> + apr_size_t old = val;
> + val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p,
> + (void *)(apr_uintptr_t)(val +
> 1),
> + (void *)(apr_uintptr_t)old);
> + if (val == old) {
> + break;
> + }
> + }
> +#endif
> +}
> +
> +PROXY_DECLARE(apr_size_t) getbusy_count(proxy_worker *worker)
> +{
> + apr_size_t val;
> +#if APR_SIZEOF_VOIDP == 4
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> + val = apr_atomic_read32(&worker->s->busy);
> +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before 1.7.4
> */
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> + val = apr_atomic_read64(&worker->s->busy);
> +#else /* Use atomics for (64bit) pointers */
> + void *volatile *busy_p = (void *)&worker->s->busy;
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> + AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> + val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> +#endif
> +
> + return val;
> +}
> +
> +PROXY_DECLARE(void) setbusy_count(proxy_worker *worker, apr_size_t to)
> +{
> +#if APR_SIZEOF_VOIDP == 4
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> + apr_atomic_set32(&worker->s->busy, to);
> +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before 1.7.4
> */
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> + apr_atomic_set64(&worker->s->busy, to);
> +#else /* Use atomics for (64bit) pointers */
> + void *volatile *busy_p = (void *)&worker->s->busy;
> + AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> + AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> + apr_atomic_xchgptr((void *)busy_p, (void *)(apr_uintptr_t)to);
> +#endif
> +}
> +
> static void add_pollset(apr_pollset_t *pollset, apr_pollfd_t *pfd,
> apr_int16_t events)
> {
>
Regards
Rüdiger
