On 11/23/23 3:53 PM, Yann Ylavic wrote:
> 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..).
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
