On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski <[email protected]> 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 <[email protected]> wrote:
>
> > On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <[email protected]> wrote:
> > On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <[email protected]> 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 <[email protected]> wrote:
> >
> > > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <[email protected]>
> wrote:
> > >
> > > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <[email protected]> wrote:
> > >
> > > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <[email protected]>
> wrote:
> > > >
> > > >
> > > > [email protected] 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/
