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/