apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Date Fri, 22 Nov 2013 23:22:02 GMT
On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski <jim@jagunet.com> wrote:

> The thing is is not only do we worry about the return code
> but also that the values we dec32 and inc32 also behave
> as signed ints. Note below we worry also that queue_info->idlers
> itself is signed, and can be < 0 :
>

Okay gurus, tell me where I'm confused (maybe I'm trainable):

I guess the opportunity for failure is if one of the APR implementations of
apr_atomic_dec32() is setting the return value in C code.  But doing that
(i.e., not having the assembly code set a local variable which is returned)
is uncommon.

Imagine that this existing z/OS code is changed to simply "return new_val".
 Conversion from 32-bit unsigned int to 32-bit signed int is undefined IIUC
and in the unlikely case that the compiler tried to change the bits some
trick would be needed in the code.

int apr_atomic_dec32(volatile apr_uint32_t *mem)
{
    apr_uint32_t old, new_val;

    old = *mem;   /* old is automatically updated on cs failure */
    do {
        new_val = old - 1;
    } while (__cs(&old, (cs_t *)mem, new_val));

    return new_val != 0;
}



> apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
> {
>     int prev_idlers;
>     prev_idlers = apr_atomic_dec32((apr_uint32_t *)&(queue_info->idlers));
>     if (prev_idlers <= 0) {
>         apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers));    /*
> back out dec */
>         return APR_EAGAIN;
>     }
>     return APR_SUCCESS;
> }
>
> apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info,
>                                           int *had_to_block)
> {
>     apr_status_t rv;
>     int 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);
>
>     /* Block if there weren't any idle workers */
>     if (prev_idlers <= 0) {
>         rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
>         if (rv != APR_SUCCESS) {
>             AP_DEBUG_ASSERT(0);
>             /* See TODO in ap_queue_info_set_idle() */
>             apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers));    /*
> back out dec */
>             return rv;
>         }
>         /* Re-check the idle worker count to guard against a
>          * race condition.  Now that we're in the mutex-protected
>          * region, one of two things may have happened:
>          *   - If the idle worker count is still negative, the
>          *     workers are all still busy, so it's safe to
>          *     block on a condition variable.
>          *   - If the idle worker count is non-negative, then a
>          *     worker has become idle since the first check
>          *     of queue_info->idlers above.  It's possible
>          *     that the worker has also signaled the condition
>          *     variable--and if so, the listener missed it
>          *     because it wasn't yet blocked on the condition
>          *     variable.  But if the idle worker count is
>          *     now non-negative, it's safe for this function to
>          *     return immediately.
>          *
>          *     A negative value in queue_info->idlers tells how many
>          *     threads are waiting on an idle worker.
>          */
>         if (queue_info->idlers < 0) {
>             *had_to_block = 1;
>             rv = apr_thread_cond_wait(queue_info->wait_for_idler,
>                                       queue_info->idlers_mutex);
>
> On Nov 22, 2013, at 3:40 PM, Jeff Trawick <trawick@gmail.com> wrote:
>
> > On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <trawick@gmail.com> wrote:
> > On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <jim@jagunet.com> wrote:
> > Note, the only think changed in event now (via
> https://svn.apache.org/viewvc?view=revision&revision=1542560)
> > is that event *checks* that atomics work as required for
> > event... if the check fails, it means that event has
> > been "broken" on that system, assuming it ever hit
> > blocked idlers, for a *long* time...
> >
> > Got it...  fdqueue.c is asking for trouble...
> >
> > I'm using atomic/unix/ia32.c with icc too.
> >
> > Need to compare generated code...  I hate stuff like "int foo() {
> unsigned char x;  ... return x;  }"
> >
> >
> > APR is documented as returning "zero if the value becomes zero on
> decrement, otherwise non-zero".
> >
> > With gcc we use get __sync_sub_and_fetch(), which returns the new value
> after the decrement.
> >
> > With icc we use the assembler implementation in APR.  I think that's
> returning 1 instead of -1.
> >
> > Here is where fdqueue needs to be able to see a negative return code:
> >
> > apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
> > {
> >     int prev_idlers;
> >     prev_idlers = apr_atomic_dec32((apr_uint32_t
> *)&(queue_info->idlers));
> >     if (prev_idlers <= 0) {
> >         apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers));    /*
> back out dec */
> >         return APR_EAGAIN;
> >     }
> >     return APR_SUCCESS;
> > }
> >
> > ("prev" in the ia32.c version of apr_atomic_dec32() and "prev_idlers"
> here is deceiving.)
> >
> >
> >
> > You should be seeing it in trunk as well...
> >
> >
> > On Nov 22, 2013, at 2:43 PM, Jeff Trawick <trawick@gmail.com> wrote:
> >
> > > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <jim@jagunet.com>
> wrote:
> > >
> > > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <trawick@gmail.com> wrote:
> > >
> > > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rpluem@apache.org>
> wrote:
> > > >
> > > >
> > > > jim@apache.org wrote:
> > > > > +        i = apr_atomic_dec32(&foo);
> > > > > +        if (i >= 0) {
> > > >
> > > > Why can we expect i < 0? apr_atomic_dec32 returns 0 if the dec
> causes foo to become zero and it returns non zero
> > > > otherwise. Shouldn't this behavior the same across all platforms?
> And if not should that be fixed in APR?
> > > >
> > > > icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb
> here.
> > > >
> > > > --enable-nonportable-atomics is specified for apr, though I haven't
> checked what that does with icc.
> > > >
> > >
> > > As noted back with the orig update, this test is due to the
> > > fdqueue code in the new event:
> > >
> > > apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info,
> > >                                     apr_pool_t * pool_to_recycle)
> > > {
> > >     apr_status_t rv;
> > >     int prev_idlers;
> > >
> > >     ap_push_pool(queue_info, pool_to_recycle);
> > >
> > >     /* Atomically increment the count of idle workers */
> > >     /*
> > >      * TODO: The atomics expect unsigned whereas we're using signed.
> > >      *       Need to double check that they work as expected or else
> > >      *       rework how we determine blocked.
> > >      * UPDATE: Correct operation is performed during open_logs()
> > >      */
> > >     prev_idlers = apr_atomic_inc32((apr_uint32_t
> *)&(queue_info->idlers));
> > >
> > >     /* If other threads are waiting on a worker, wake one up */
> > >     if (prev_idlers < 0) {
> > >
> > >
> > > See the comments ("The atomics expect unsigned whereas...") for
> > > the reason, etc.
> > >
> > > When you say "icc (Intel) builds of httpd 2.4.7 event MPM (with
> apr-1.5.0) bomb here."
> > > do you mean that you get the 'atomics not working as expected' error
> > > (and the internal server error) or that it core dumps?
> > >
> > >
> > > "atomics not working as expected"
> > >
> > > Let me see what code is used...
> > >
> > > --
> > > Born in Roswell... married an alien...
> > > http://emptyhammock.com/
> >
> >
> >
> >
> > --
> > Born in Roswell... married an alien...
> > http://emptyhammock.com/
> >
> >
> >
> > --
> > Born in Roswell... married an alien...
> > http://emptyhammock.com/
>
>


-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Mime
View raw message