Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Ruediger Pluem]

On 11/28/23 10:29 PM, Yann Ylavic wrote:

> 
> Tried something in https://github.com/apache/httpd/pull/396
> WDYT?

Looks good. Thanks.

Regards

Rüdiger

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Jim Jagielski]

> On Nov 28, 2023, at 4:29 PM, Yann Ylavic  wrote:
> 
> On Fri, Nov 24, 2023 at 3:28 PM Ruediger Pluem  > wrote:
>> 
>> On 11/24/23 10:59 AM, Yann Ylavic wrote:
>>> On Thu, Nov 23, 2023 at 5:47 PM Ruediger Pluem  wrote:
 
 On 11/23/23 5:05 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
>> 
>> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
>>> 
>>> On 11/23/23 3:53 PM, Yann Ylavic wrote:
 
>>> I am asking because I guess I am hit now by races in the byrequests LB
>>> with the worker->s->lbstatus.
>>> I probably want to look for a way to solve this in a more generic way
>>> for various shared worker fields.
>> 
>> lbstatus is an int so the 32bit apr_atomic API could do I think.
> 
> I mean, if lbstatus becomes inconsistent because of the race then we
 
 My current theory is that this is the case.
 
> can do something, but if it is e.g. that a worker switches from error
> state to non-error become some threads can connect while some others
> can't this is something we should address elsewhere (like a failure
> threshold).
> 
>> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
>> system dependent widths. It preferably would be implemented in APR but
>> in the meantime we could have something in httpd already.
>> What we should avoid IMO is needing 64bit atomics on 32bit systems
>> (because we'd have to reimplement the mutexes etc), but apart from
>> that I think we can wrap anything using the existing apr atomics.
> 
> What we need for lbstatus is ap_atomic_int_or() and
> ap_atomic_int_and_not() it seems, both could be implemented with a
> cas32 loop.
 
 What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are 
 supposed to deliver?
 I am having a hard time guessing it from the name :-).
>>> 
>>> Actually I mixed ->lbstatus and ->status, so I thought we needed an
>>> atomic "or" and an atomic "and not" to handle the ->status mask..
>>> 
>>> But ->lbstatus is much like the ->busy count and updated almost at the
>>> same place, though it's an "int" and we can't reuse the same functions
>>> for both :/
>>> It seems that we need to make sure that 0 <= lbstatus <= INT_MAX too,
>> 
>> I think lbstatus is allowed to become negative.
> 
> Tried something in https://github.com/apache/httpd/pull/396
> WDYT?

+1 for concept

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Fri, Nov 24, 2023 at 3:28 PM Ruediger Pluem  wrote:
>
> On 11/24/23 10:59 AM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 5:47 PM Ruediger Pluem  wrote:
> >>
> >> On 11/23/23 5:05 PM, Yann Ylavic wrote:
> >>> On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
> 
>  On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
> >
> > On 11/23/23 3:53 PM, Yann Ylavic wrote:
> >>
> > I am asking because I guess I am hit now by races in the byrequests LB
> > with the worker->s->lbstatus.
> > I probably want to look for a way to solve this in a more generic way
> > for various shared worker fields.
> 
>  lbstatus is an int so the 32bit apr_atomic API could do I think.
> >>>
> >>> I mean, if lbstatus becomes inconsistent because of the race then we
> >>
> >> My current theory is that this is the case.
> >>
> >>> can do something, but if it is e.g. that a worker switches from error
> >>> state to non-error become some threads can connect while some others
> >>> can't this is something we should address elsewhere (like a failure
> >>> threshold).
> >>>
>  Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
>  system dependent widths. It preferably would be implemented in APR but
>  in the meantime we could have something in httpd already.
>  What we should avoid IMO is needing 64bit atomics on 32bit systems
>  (because we'd have to reimplement the mutexes etc), but apart from
>  that I think we can wrap anything using the existing apr atomics.
> >>>
> >>> What we need for lbstatus is ap_atomic_int_or() and
> >>> ap_atomic_int_and_not() it seems, both could be implemented with a
> >>> cas32 loop.
> >>
> >> What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are 
> >> supposed to deliver?
> >> I am having a hard time guessing it from the name :-).
> >
> > Actually I mixed ->lbstatus and ->status, so I thought we needed an
> > atomic "or" and an atomic "and not" to handle the ->status mask..
> >
> > But ->lbstatus is much like the ->busy count and updated almost at the
> > same place, though it's an "int" and we can't reuse the same functions
> > for both :/
> > It seems that we need to make sure that 0 <= lbstatus <= INT_MAX too,
>
> I think lbstatus is allowed to become negative.

Tried something in https://github.com/apache/httpd/pull/396
WDYT?

Regards;
Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Ruediger Pluem]

On 11/24/23 10:59 AM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 5:47 PM Ruediger Pluem  wrote:
>>
>> On 11/23/23 5:05 PM, Yann Ylavic wrote:
>>> On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:

 On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
>
> On 11/23/23 3:53 PM, Yann Ylavic wrote:
>>
> I am asking because I guess I am hit now by races in the byrequests LB
> with the worker->s->lbstatus.
> I probably want to look for a way to solve this in a more generic way
> for various shared worker fields.

 lbstatus is an int so the 32bit apr_atomic API could do I think.
>>>
>>> I mean, if lbstatus becomes inconsistent because of the race then we
>>
>> My current theory is that this is the case.
>>
>>> can do something, but if it is e.g. that a worker switches from error
>>> state to non-error become some threads can connect while some others
>>> can't this is something we should address elsewhere (like a failure
>>> threshold).
>>>
 Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
 system dependent widths. It preferably would be implemented in APR but
 in the meantime we could have something in httpd already.
 What we should avoid IMO is needing 64bit atomics on 32bit systems
 (because we'd have to reimplement the mutexes etc), but apart from
 that I think we can wrap anything using the existing apr atomics.
>>>
>>> What we need for lbstatus is ap_atomic_int_or() and
>>> ap_atomic_int_and_not() it seems, both could be implemented with a
>>> cas32 loop.
>>
>> What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are 
>> supposed to deliver?
>> I am having a hard time guessing it from the name :-).
> 
> Actually I mixed ->lbstatus and ->status, so I thought we needed an
> atomic "or" and an atomic "and not" to handle the ->status mask..
> 
> But ->lbstatus is much like the ->busy count and updated almost at the
> same place, though it's an "int" and we can't reuse the same functions
> for both :/
> It seems that we need to make sure that 0 <= lbstatus <= INT_MAX too,

I think lbstatus is allowed to become negative.


Regards

Rüdiger

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 5:47 PM Ruediger Pluem  wrote:
>
> On 11/23/23 5:05 PM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
> >>
> >> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
> >>>
> >>> On 11/23/23 3:53 PM, Yann Ylavic wrote:
>
> >>> I am asking because I guess I am hit now by races in the byrequests LB
> >>> with the worker->s->lbstatus.
> >>> I probably want to look for a way to solve this in a more generic way
> >>> for various shared worker fields.
> >>
> >> lbstatus is an int so the 32bit apr_atomic API could do I think.
> >
> > I mean, if lbstatus becomes inconsistent because of the race then we
>
> My current theory is that this is the case.
>
> > can do something, but if it is e.g. that a worker switches from error
> > state to non-error become some threads can connect while some others
> > can't this is something we should address elsewhere (like a failure
> > threshold).
> >
> >> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
> >> system dependent widths. It preferably would be implemented in APR but
> >> in the meantime we could have something in httpd already.
> >> What we should avoid IMO is needing 64bit atomics on 32bit systems
> >> (because we'd have to reimplement the mutexes etc), but apart from
> >> that I think we can wrap anything using the existing apr atomics.
> >
> > What we need for lbstatus is ap_atomic_int_or() and
> > ap_atomic_int_and_not() it seems, both could be implemented with a
> > cas32 loop.
>
> What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are 
> supposed to deliver?
> I am having a hard time guessing it from the name :-).

Actually I mixed ->lbstatus and ->status, so I thought we needed an
atomic "or" and an atomic "and not" to handle the ->status mask..

But ->lbstatus is much like the ->busy count and updated almost at the
same place, though it's an "int" and we can't reuse the same functions
for both :/
It seems that we need to make sure that 0 <= lbstatus <= INT_MAX too,
and later we'll possibly want atomic add/set for ->transferred and
->read used by bytraffic which are off_t..

So the abstraction I'm seeing is:
  PROXY_DECLARE(int) ap_atomic_int_get(volatile int *val);
  PROXY_DECLARE(void) ap_atomic_int_set(volatile int *val, int to);
  PROXY_DECLARE(int) ap_atomic_int_sub_not_min(volatile int *val, int
sub, int min);
  PROXY_DECLARE(int) ap_atomic_int_add_not_max(volatile int *val, int
add, int max);
which all return the old *val (maybe ap_atomic_int_set() too).
The same for uint, long, ulong, and then point size_t/off_t to them
depending on their width. We'd implement the ones needed as we use
them only, but note that if we need off_t (or longlong or ulonglong)
at some point we possibly want to require a 64bit system for them (as
I said earlier).
Finally we'd use apr_atomic_int_* for ->lbstatus, apr_atomic_size[t]_*
for ->busy etc.

So it's an abstraction but not really a short one to write, should be
quite mechanical though with the same code structure as what we have
in this commit (possibly we can use macros for the implementation).
Once we have that, it could be useful for APR too, though there we'd
rather point the system types to the 32/64bits base ones.
Sorry I can't think of both generic and simpler, it's still better
than ap_proxy_{get,set,sub,add}_{many_proxy_worker_shared_fields}()
IMO.

Regards;
Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Ruediger Pluem]

On 11/23/23 5:05 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
>>
>> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
>>>
>>> On 11/23/23 3:53 PM, Yann Ylavic wrote:

>>> I am asking because I guess I am hit now by races in the byrequests LB
>>> with the worker->s->lbstatus.
>>> I probably want to look for a way to solve this in a more generic way
>>> for various shared worker fields.
>>
>> lbstatus is an int so the 32bit apr_atomic API could do I think.
> 
> I mean, if lbstatus becomes inconsistent because of the race then we

My current theory is that this is the case.

> can do something, but if it is e.g. that a worker switches from error
> state to non-error become some threads can connect while some others
> can't this is something we should address elsewhere (like a failure
> threshold).
> 
>> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
>> system dependent widths. It preferably would be implemented in APR but
>> in the meantime we could have something in httpd already.
>> What we should avoid IMO is needing 64bit atomics on 32bit systems
>> (because we'd have to reimplement the mutexes etc), but apart from
>> that I think we can wrap anything using the existing apr atomics.
> 
> What we need for lbstatus is ap_atomic_int_or() and
> ap_atomic_int_and_not() it seems, both could be implemented with a
> cas32 loop.

What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are 
supposed to deliver?
I am having a hard time guessing it from the name :-).

Regards

Rüdiger

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
>
> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
> >
> > On 11/23/23 3:53 PM, Yann Ylavic wrote:
> > >
> > > 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..
> >
> > Understood.
> >
> > I am asking because I guess I am hit now by races in the byrequests LB
> > with the worker->s->lbstatus.
> > I probably want to look for a way to solve this in a more generic way
> > for various shared worker fields.
>
> lbstatus is an int so the 32bit apr_atomic API could do I think.

I mean, if lbstatus becomes inconsistent because of the race then we
can do something, but if it is e.g. that a worker switches from error
state to non-error become some threads can connect while some others
can't this is something we should address elsewhere (like a failure
threshold).

> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
> system dependent widths. It preferably would be implemented in APR but
> in the meantime we could have something in httpd already.
> What we should avoid IMO is needing 64bit atomics on 32bit systems
> (because we'd have to reimplement the mutexes etc), but apart from
> that I think we can wrap anything using the existing apr atomics.

What we need for lbstatus is ap_atomic_int_or() and
ap_atomic_int_and_not() it seems, both could be implemented with a
cas32 loop.

>
> Regards;
> Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
>
> On 11/23/23 3:53 PM, Yann Ylavic wrote:
> >
> > 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..
>
> Understood.
>
> I am asking because I guess I am hit now by races in the byrequests LB
> with the worker->s->lbstatus.
> I probably want to look for a way to solve this in a more generic way
> for various shared worker fields.

lbstatus is an int so the 32bit apr_atomic API could do I think.
Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
system dependent widths. It preferably would be implemented in APR but
in the meantime we could have something in httpd already.
What we should avoid IMO is needing 64bit atomics on 32bit systems
(because we'd have to reimplement the mutexes etc), but apart from
that I think we can wrap anything using the existing apr atomics.

Regards;
Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Ruediger Pluem]

On 11/23/23 3:53 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 12:11 PM Ruediger Pluem  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=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() < 0 then atomic_inc()"
> 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..).

Thanks for the pointer. I missed this.

> 
> 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..

Understood.

I am asking because I guess I am hit now by races in the byrequests LB
with the worker->s->lbstatus.
I probably want to look for a way to solve this in a more generic way
for various shared worker fields.

Regards

Rüdiger

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 12:11 PM Ruediger Pluem  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=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() < 0 then atomic_inc()"
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(>s->busy);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas32(>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(>s->busy);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas64(>s->busy, val - 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#else /* Use atomics for (64bit) pointers */
> > +void *volatile *busy_p = (void *)>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(>s->busy);
> > +while (val < APR_INT32_MAX) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas32(>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(>s->busy);
> > +while (val < APR_INT64_MAX) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas64(>s->busy, val + 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#else /* Use atomics for (64bit) pointers */
> > +void *volatile *busy_p = (void *)>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,
> > +   

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Ruediger Pluem]

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=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=1912244=1912245=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
>"\n", NULL);
>  ap_rprintf(r,
> "  %" APR_SIZE_T_FMT 
> "\n",
> -   worker->s->busy);
> +   getbusy_count(worker));
>  ap_rprintf(r, "  %d\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("", r);
>  ap_rprintf(r, "%" APR_SIZE_T_FMT "", 
> worker->s->elected);
> -ap_rprintf(r, "%" APR_SIZE_T_FMT "", 
> worker->s->busy);
> +ap_rprintf(r, "%" APR_SIZE_T_FMT "", 
> getbusy_count(worker));
>  ap_rprintf(r, "%d", worker->s->lbstatus);
>  ap_rputs(apr_strfsize(worker->s->transferred, fbuf), r);
>  ap_rputs("", 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=1912244=1912245=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(>s->busy);
> +while (val > 0) {
> +apr_size_t old = val;
> +val = apr_atomic_cas32(>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(>s->busy);
> +while (val > 0) {
> +apr_size_t old = val;
> +val = apr_atomic_cas64(>s->busy, val - 1, old);
> +if (val == old) {
> +break;
> +}
> +}
> +#else /* Use atomics for (64bit) pointers */
> +void *volatile *busy_p = (void *)>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) {
> +