On Thu, Nov 23, 2023 at 12:11 PM Ruediger Pluem <rpl...@apache.org> wrote: > > 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 [] > > --- 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?
This has been discussed in https://github.com/apache/httpd/pull/383, at least for why the "if atomic_dec(&busy) < 0 then atomic_inc(&busy)" proposed originally was racy. So the below implements dec_not_zero and inc_not_at_max to keep ->busy in the bounds, like the original non-atomic functions. I think the under/overflows could still happen without this because, even if we inc per request and dec on request pool cleanup, there is a balancer->lbmethod->reset() which may clear ->busy at any time (IIUC), not to talk about a child crash (though then ->busy may not be very reliable anyway, under/overflow or not..). As for the 64bit atomics vs APR version dance, it's because the former are not available before apr-1.7 and likely not reliable before 1.7.4 (at least for some architectures where builtins are not available). In any case we need a fallback for apr < 1.7, but maybe to keep this simpler we should fall back to non-atomics (as before) in this case. It all looks over complicated for the feature, but I can't think of something simpler and still correct.. > > > +{ > > + 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 > > +} Yeah it's not nice :/ Regards; Yann.
