httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1545286 - in /httpd/httpd/trunk/server/mpm/event: event.c fdqueue.c
Date Mon, 25 Nov 2013 16:53:25 GMT
On Mon, Nov 25, 2013 at 4:53 PM, Jim Jagielski <jim@jagunet.com> wrote:

>
> On Nov 25, 2013, at 10:24 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> >
> > As Jeff said in the other thread, I think the test here should be :
> >    if (prev_idlers <= 1)
> > that's because dec32 was returning the new value while add32 now returns
> the old one (fetch_and_sub vs sub_and_fetch).
> >
>
> We aren't being consistent.. see below:
>
> > +static const apr_uint32_t zero_pt = ((apr_uint32_t)1 << 31);
>
> Hmmmm... for a 32bit int, shouldn't that be << 29 ??
>

I don't see, why 29?
Suppose idlers is 0: idlers - zero_pt == 0 - 2^31 == INT32_MIN;
and when idlers is 2^32-1: (2^32-1) - 2^31 == INT32_MAX.
Is there a need for more positives than negatives?


>
> >
> >  apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
> >  {
> > -    int prev_idlers;
> > -    prev_idlers = apr_atomic_add32((apr_uint32_t
> *)&(queue_info->idlers), -1) - zero_pt;
> > -    if (prev_idlers <= 0) {
>
> Note the check here....
>
> > -        apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers));    /*
> back out dec */
> > +    apr_int32_t prev_idlers;
> > +    prev_idlers = apr_atomic_add32(&queue_info->idlers, -1) - zero_pt;
> > +    if (prev_idlers <= 1) {
> > +        apr_atomic_inc32(&queue_info->idlers);    /* back out dec */
> >          return APR_EAGAIN;
> >      }
> >      return APR_SUCCESS;
> > @@ -140,11 +142,11 @@ apr_status_t ap_queue_info_wait_for_idler(fd_queue
> >                                            int *had_to_block)
> >  {
> >      apr_status_t rv;
> > -    int prev_idlers;
> > +    apr_int32_t prev_idlers;
> >
> >      /* Atomically decrement the idle worker count, saving the old value
> */
> >      /* See TODO in ap_queue_info_set_idle() */
> > -    prev_idlers = apr_atomic_add32((apr_uint32_t
> *)&(queue_info->idlers), -1) - zero_pt;
> > +    prev_idlers = apr_atomic_add32(&queue_info->idlers, -1) - zero_pt;
> >
> >      /* Block if there weren't any idle workers */
> >      if (prev_idlers <= 0) {
>
> And here....
>

Indeed, that was not consistent!
This is a second fix then, the check was doubly broken...

Mime
View raw message