On Thu, Nov 23, 2023 at 5:47 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 11/23/23 5:05 PM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic <ylavic....@gmail.com> wrote:
> >>
> >> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem <rpl...@apache.org> 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.
