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 :
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/
