On Fri, Nov 24, 2023 at 3:28 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 11/24/23 10:59 AM, Yann Ylavic wrote:
> > 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,
>
> I think lbstatus is allowed to become negative.
Tried something in https://github.com/apache/httpd/pull/396
WDYT?
Regards;
Yann.
