On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski <j...@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 <traw...@gmail.com> wrote: > > > On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <traw...@gmail.com> wrote: > > On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <j...@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 <traw...@gmail.com> wrote: > > > > > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <j...@jagunet.com> > wrote: > > > > > > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <traw...@gmail.com> wrote: > > > > > > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rpl...@apache.org> > wrote: > > > > > > > > > > > > j...@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/