You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2012/11/17 12:00:05 UTC
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
jim@apache.org wrote:
> Author: jim
> Date: Fri Nov 16 16:49:31 2012
> New Revision: 1410459
>
> URL: http://svn.apache.org/viewvc?rev=1410459&view=rev
> Log:
> fdq expects a certain behavior from atomics... ensure that
> the event mpms check this.
>
> Modified:
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/server/mpm/event/event.c
> httpd/httpd/trunk/server/mpm/eventopt/eventopt.c
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1410459&r1=1410458&r2=1410459&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Nov 16 16:49:31 2012
> @@ -2928,6 +2928,18 @@ static int event_pre_config(apr_pool_t *
> }
> ++retained->module_loads;
> if (retained->module_loads == 2) {
> + int i;
> + static apr_uint32_t foo = 0;
> +
> + apr_atomic_inc32(&foo);
> + apr_atomic_dec32(&foo);
> + apr_atomic_dec32(&foo);
> + 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?
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL, APLOGNO(02405)
> + "atomics not working as expected");
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
> rv = apr_pollset_create(&event_pollset, 1, plog,
> APR_POLLSET_THREADSAFE | APR_POLLSET_NOCOPY);
> if (rv != APR_SUCCESS) {
>
>
Regards
Rüdiger
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
>
> > On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
> > On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
> >
> > > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
> wrote:
> > >
> > > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
> > >
> > > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
>
> > On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
> > On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
> >
> > > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
> wrote:
> > >
> > > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
> > >
> > > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
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 <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
>
> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> >
> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
> >
> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Mon, Nov 25, 2013 at 1:27 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
> On Nov 24, 2013, at 7:18 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
> > On Sat, Nov 23, 2013 at 5:39 PM, Yann Ylavic <yl...@gmail.com>
> wrote:
> > Couldn't ap_queue_info_try_get_idler() and the event_pre_config() check
> use :
> >
> >
> > prev_idlers = apr_atomic_add32((apr_uint32_t
> *)&(queue_info->idlers), -1);
> >
> > like ap_queue_info_wait_for_idler() does ?
> >
> > I think that's correct...
> >
> > What the code really wants is new_idlers, so edit that slightly to
> >
> > new_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers),
> -1) - 1;
> > if (new_idlers <= 0) {
> > ...
> > return APR_EAGAIN;
> > }
>
> It just wants to see if there are any idle ones... If there wasn't
> before the dec, then we need to return APR_EAGAIN. Recall that
> a 0 means "no idlers" and neg means 'this many blocking', and
> the logic is the same in both cases, minus the "wake up" scenario.
>
>
I think this is a safe progression from the original code, which was:
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) {
We know now that ALMOST every apr_atomic_dec32() returned the new idlers
value, and none returned the previous value, so fix the variable name to
reflect reality where it worked:
apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
int new_idlers;
new_idlers = apr_atomic_dec32((apr_uint32_t *)&(queue_info->idlers));
if (new_idlers <= 0) {
You can't simply replace apr_atomic_dec32(foo) with apr_atomic_add32(foo,
-1) since the latter returns the old value instead of the new value, so you
have to subtract 1 from the return code of apr_atomic_dec32() to set
new_idlers to the same result as before.
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 24, 2013, at 7:18 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Sat, Nov 23, 2013 at 5:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Couldn't ap_queue_info_try_get_idler() and the event_pre_config() check use :
>
>
> prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), -1);
>
> like ap_queue_info_wait_for_idler() does ?
>
> I think that's correct...
>
> What the code really wants is new_idlers, so edit that slightly to
>
> new_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), -1) - 1;
> if (new_idlers <= 0) {
> ...
> return APR_EAGAIN;
> }
It just wants to see if there are any idle ones... If there wasn't
before the dec, then we need to return APR_EAGAIN. Recall that
a 0 means "no idlers" and neg means 'this many blocking', and
the logic is the same in both cases, minus the "wake up" scenario.
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Nov 23, 2013 at 5:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Couldn't ap_queue_info_try_get_idler() and the event_pre_config() check
> use :
>
>
> prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers),
> -1);
>
> like ap_queue_info_wait_for_idler() does ?
>
I think that's correct...
What the code really wants is new_idlers, so edit that slightly to
new_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), -1) -
1;
if (new_idlers <= 0) {
...
return APR_EAGAIN;
}
>
> Or maybe queue_info->idlers be declared uint32_t and negatives computed
> relative to 2^31 ?
>
The comments say that the uint/int discrepancy is the problem, but the
actual problem is the expectation that apr_atomic_dec32() returns the new
value, when all that is promised is a zero/non-zero flag. The new value is
what is returned in almost all implementations, but that is not actually
promised by the API, and promising new value (perhaps in apr-future) is
rather ugly with a declared return type of int.
> Regards,
> Yann.
>
>
>
>
> On Fri, Nov 22, 2013 at 9:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>
>>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
>>>>
>>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>>> wrote:
>>>> >
>>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>> >
>>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 23, 2013, at 5:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Or maybe queue_info->idlers be declared uint32_t and negatives computed relative to 2^31 ?
>
What I was thinking was simply doing an offset... There
is no way we would ever use a full 32bits, signed or not,
so making the "zero point" ~ 2^31 would allow us to keep
the logic, without worrying about the behavior of
atomics.
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Yann Ylavic <yl...@gmail.com>.
Couldn't ap_queue_info_try_get_idler() and the event_pre_config() check use
:
prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers),
-1);
like ap_queue_info_wait_for_idler() does ?
Or maybe queue_info->idlers be declared uint32_t and negatives computed
relative to 2^31 ?
Regards,
Yann.
On Fri, Nov 22, 2013 at 9:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
>>>
>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>> wrote:
>>> >
>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>> >
>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
>
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 22.11.2013 23:03, Jim Jagielski wrote:
> Anyone ever try OpenPA?
>
> https://trac.mcs.anl.gov/projects/openpa/
>
> It's under MIT, fwiw.
Haven't tried it but the README
http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
indicates only platform support based on gcc plus Windows (but they say
they haven't set up a build system for it) plus Solaris (but that seems
to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
have the needed implementation).
Regards,
Rainer
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sun, Nov 24, 2013 at 7:33 AM, Eric Covener <co...@gmail.com> wrote:
> On Sun, Nov 24, 2013 at 5:24 AM, Rainer Jung <ra...@kippdata.de>
> wrote:
> > On 24.11.2013 01:03, Eric Covener wrote:
> >>>> I'm curious what other scenarios will fail though. I can try Sun
> Studio on
> >>>> Solaris 10 x86_64 (32-bit and 64-bit builds) "soon". But Sun Studio
> on
> >>>> SPARC presumably uses different explicit code in APR and I don't have
> access
> >>>> to that.
> >>>
> >>> I will try to get that one up and running on 2.4.7.
> >>
> >> No startup error with 2.4.7 + event on sparc32 or sparc64 with sun
> >> studio 12 (cc 5.9)
> >
> > For the sake of completeness: was apr build with the nonportable atomics
> > switch enabled? Don't know whether that really matters, but want to make
> > sure we understand the test case.
>
> Defaults before, retested with no difference under
> --enable-nonportable-atomics
>
Solaris 10 on Intel/AMD, Sun Studio, --enable-nonportable-atomics, 32-bit
and 64-bit, 2.4.x HEAD, apr 1.5.x HEAD, apr-util 1.5.x HEAD
no problem; uses Solaris-specific atomic implementation, which uses the
return-new-value version of decrement
The portable, "mutex" version of atomics clearly returns the new value too,
though I didn't test with it.
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Eric Covener <co...@gmail.com>.
On Sun, Nov 24, 2013 at 5:24 AM, Rainer Jung <ra...@kippdata.de> wrote:
> On 24.11.2013 01:03, Eric Covener wrote:
>>>> I'm curious what other scenarios will fail though. I can try Sun Studio on
>>>> Solaris 10 x86_64 (32-bit and 64-bit builds) "soon". But Sun Studio on
>>>> SPARC presumably uses different explicit code in APR and I don't have access
>>>> to that.
>>>
>>> I will try to get that one up and running on 2.4.7.
>>
>> No startup error with 2.4.7 + event on sparc32 or sparc64 with sun
>> studio 12 (cc 5.9)
>
> For the sake of completeness: was apr build with the nonportable atomics
> switch enabled? Don't know whether that really matters, but want to make
> sure we understand the test case.
Defaults before, retested with no difference under --enable-nonportable-atomics
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 24.11.2013 01:03, Eric Covener wrote:
>>> I'm curious what other scenarios will fail though. I can try Sun Studio on
>>> Solaris 10 x86_64 (32-bit and 64-bit builds) "soon". But Sun Studio on
>>> SPARC presumably uses different explicit code in APR and I don't have access
>>> to that.
>>
>> I will try to get that one up and running on 2.4.7.
>
> No startup error with 2.4.7 + event on sparc32 or sparc64 with sun
> studio 12 (cc 5.9)
For the sake of completeness: was apr build with the nonportable atomics
switch enabled? Don't know whether that really matters, but want to make
sure we understand the test case.
Regards,
Rainer
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Eric Covener <co...@gmail.com>.
>> I'm curious what other scenarios will fail though. I can try Sun Studio on
>> Solaris 10 x86_64 (32-bit and 64-bit builds) "soon". But Sun Studio on
>> SPARC presumably uses different explicit code in APR and I don't have access
>> to that.
>
> I will try to get that one up and running on 2.4.7.
No startup error with 2.4.7 + event on sparc32 or sparc64 with sun
studio 12 (cc 5.9)
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Eric Covener <co...@gmail.com>.
On Sat, Nov 23, 2013 at 3:56 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Sat, Nov 23, 2013 at 3:45 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>> I'm just curious why people never saw this on trunk...
>> That code path is almost exactly a year old. No one has
>> hit this in 12 months??
>
>
> l rarely try trunk except on
>
> * Windows, which uses a system mechanism regardless of compiler
> * Linux or FreeBSD, 64-bit gcc builds
>
> --/--
>
> I'm curious what other scenarios will fail though. I can try Sun Studio on
> Solaris 10 x86_64 (32-bit and 64-bit builds) "soon". But Sun Studio on
> SPARC presumably uses different explicit code in APR and I don't have access
> to that.
I will try to get that one up and running on 2.4.7.
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Nov 23, 2013 at 3:45 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> I'm just curious why people never saw this on trunk...
> That code path is almost exactly a year old. No one has
> hit this in 12 months??
>
l rarely try trunk except on
* Windows, which uses a system mechanism regardless of compiler
* Linux or FreeBSD, 64-bit gcc builds
--/--
I'm curious what other scenarios will fail though. I can try Sun Studio on
Solaris 10 x86_64 (32-bit and 64-bit builds) "soon". But Sun Studio on
SPARC presumably uses different explicit code in APR and I don't have
access to that.
Right now it looks reasonable to change the ia32.c and os390/atomic.c
implementations in APR as described before, but there's no substitute for
actually trying the other atomic implementations, which look safe to me but
with little certainty.
> On Nov 23, 2013, at 12:45 PM, Rainer Jung <ra...@kippdata.de> wrote:
>
> > On 23.11.2013 14:15, Jeff Trawick wrote:
> >> On Sat, Nov 23, 2013 at 8:04 AM, Rainer Jung <rainer.jung@kippdata.de
> >> <ma...@kippdata.de>> wrote:
> >>
> >> On 22.11.2013 23:03, Jim Jagielski wrote:
> >>> Anyone ever try OpenPA?
> >>>
> >>> https://trac.mcs.anl.gov/projects/openpa/
> >>>
> >>> It's under MIT, fwiw.
> >>
> >> Haven't tried it but the README
> >>
> >> http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
> >>
> >> indicates only platform support based on gcc plus Windows (but they
> say
> >> they haven't set up a build system for it) plus Solaris (but that
> seems
> >> to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
> >> have the needed implementation).
> >>
> >> Regards,
> >>
> >> Rainer
> >>
> >>
> >> By the way, Rainer, in your l*m*n test combinations, which compiler(s)
> >> did you use on Solaris while confirming that 2.4.7 Event works at least
> >> a little?
> >
> > I'm only building with gcc. On Solaris 8 with gcc 4.1.2, on Solaris 10
> > the latest and greatest (4.8.2).
> >
> > Will post the full 2.4.7 results a bit later (only for the sake of
> > completeness), but the builds are also broken for event on Linux 32 Bits
> > even when using gcc:
> >
> > [Thu Nov 21 11:34:53.149765 2013] [mpm_event:crit] [pid 18492:tid
> > 3080439472] AH02405: atomics not working as expected
> >
> > Sorry for not having checked progress during the tests, but the last
> > combinations just finished right now.
> >
> > Regards,
> >
> > Rainer
> >
>
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Nov 23, 2013 at 3:45 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> I'm just curious why people never saw this on trunk...
> That code path is almost exactly a year old. No one has
> hit this in 12 months??
>
l rarely try trunk except on
* Windows, which uses a system mechanism regardless of compiler
* Linux or FreeBSD, 64-bit gcc builds
--/--
I'm curious what other scenarios will fail though. I can try Sun Studio on
Solaris 10 x86_64 (32-bit and 64-bit builds) "soon". But Sun Studio on
SPARC presumably uses different explicit code in APR and I don't have
access to that.
Right now it looks reasonable to change the ia32.c and os390/atomic.c
implementations in APR as described before, but there's no substitute for
actually trying the other atomic implementations, which look safe to me but
with little certainty.
> On Nov 23, 2013, at 12:45 PM, Rainer Jung <ra...@kippdata.de> wrote:
>
> > On 23.11.2013 14:15, Jeff Trawick wrote:
> >> On Sat, Nov 23, 2013 at 8:04 AM, Rainer Jung <rainer.jung@kippdata.de
> >> <ma...@kippdata.de>> wrote:
> >>
> >> On 22.11.2013 23:03, Jim Jagielski wrote:
> >>> Anyone ever try OpenPA?
> >>>
> >>> https://trac.mcs.anl.gov/projects/openpa/
> >>>
> >>> It's under MIT, fwiw.
> >>
> >> Haven't tried it but the README
> >>
> >> http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
> >>
> >> indicates only platform support based on gcc plus Windows (but they
> say
> >> they haven't set up a build system for it) plus Solaris (but that
> seems
> >> to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
> >> have the needed implementation).
> >>
> >> Regards,
> >>
> >> Rainer
> >>
> >>
> >> By the way, Rainer, in your l*m*n test combinations, which compiler(s)
> >> did you use on Solaris while confirming that 2.4.7 Event works at least
> >> a little?
> >
> > I'm only building with gcc. On Solaris 8 with gcc 4.1.2, on Solaris 10
> > the latest and greatest (4.8.2).
> >
> > Will post the full 2.4.7 results a bit later (only for the sake of
> > completeness), but the builds are also broken for event on Linux 32 Bits
> > even when using gcc:
> >
> > [Thu Nov 21 11:34:53.149765 2013] [mpm_event:crit] [pid 18492:tid
> > 3080439472] AH02405: atomics not working as expected
> >
> > Sorry for not having checked progress during the tests, but the last
> > combinations just finished right now.
> >
> > Regards,
> >
> > Rainer
> >
>
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
I'm just curious why people never saw this on trunk...
That code path is almost exactly a year old. No one has
hit this in 12 months??
On Nov 23, 2013, at 12:45 PM, Rainer Jung <ra...@kippdata.de> wrote:
> On 23.11.2013 14:15, Jeff Trawick wrote:
>> On Sat, Nov 23, 2013 at 8:04 AM, Rainer Jung <rainer.jung@kippdata.de
>> <ma...@kippdata.de>> wrote:
>>
>> On 22.11.2013 23:03, Jim Jagielski wrote:
>>> Anyone ever try OpenPA?
>>>
>>> https://trac.mcs.anl.gov/projects/openpa/
>>>
>>> It's under MIT, fwiw.
>>
>> Haven't tried it but the README
>>
>> http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
>>
>> indicates only platform support based on gcc plus Windows (but they say
>> they haven't set up a build system for it) plus Solaris (but that seems
>> to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
>> have the needed implementation).
>>
>> Regards,
>>
>> Rainer
>>
>>
>> By the way, Rainer, in your l*m*n test combinations, which compiler(s)
>> did you use on Solaris while confirming that 2.4.7 Event works at least
>> a little?
>
> I'm only building with gcc. On Solaris 8 with gcc 4.1.2, on Solaris 10
> the latest and greatest (4.8.2).
>
> Will post the full 2.4.7 results a bit later (only for the sake of
> completeness), but the builds are also broken for event on Linux 32 Bits
> even when using gcc:
>
> [Thu Nov 21 11:34:53.149765 2013] [mpm_event:crit] [pid 18492:tid
> 3080439472] AH02405: atomics not working as expected
>
> Sorry for not having checked progress during the tests, but the last
> combinations just finished right now.
>
> Regards,
>
> Rainer
>
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
I'm just curious why people never saw this on trunk...
That code path is almost exactly a year old. No one has
hit this in 12 months??
On Nov 23, 2013, at 12:45 PM, Rainer Jung <ra...@kippdata.de> wrote:
> On 23.11.2013 14:15, Jeff Trawick wrote:
>> On Sat, Nov 23, 2013 at 8:04 AM, Rainer Jung <rainer.jung@kippdata.de
>> <ma...@kippdata.de>> wrote:
>>
>> On 22.11.2013 23:03, Jim Jagielski wrote:
>>> Anyone ever try OpenPA?
>>>
>>> https://trac.mcs.anl.gov/projects/openpa/
>>>
>>> It's under MIT, fwiw.
>>
>> Haven't tried it but the README
>>
>> http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
>>
>> indicates only platform support based on gcc plus Windows (but they say
>> they haven't set up a build system for it) plus Solaris (but that seems
>> to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
>> have the needed implementation).
>>
>> Regards,
>>
>> Rainer
>>
>>
>> By the way, Rainer, in your l*m*n test combinations, which compiler(s)
>> did you use on Solaris while confirming that 2.4.7 Event works at least
>> a little?
>
> I'm only building with gcc. On Solaris 8 with gcc 4.1.2, on Solaris 10
> the latest and greatest (4.8.2).
>
> Will post the full 2.4.7 results a bit later (only for the sake of
> completeness), but the builds are also broken for event on Linux 32 Bits
> even when using gcc:
>
> [Thu Nov 21 11:34:53.149765 2013] [mpm_event:crit] [pid 18492:tid
> 3080439472] AH02405: atomics not working as expected
>
> Sorry for not having checked progress during the tests, but the last
> combinations just finished right now.
>
> Regards,
>
> Rainer
>
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 23.11.2013 14:15, Jeff Trawick wrote:
> On Sat, Nov 23, 2013 at 8:04 AM, Rainer Jung <rainer.jung@kippdata.de
> <ma...@kippdata.de>> wrote:
>
> On 22.11.2013 23:03, Jim Jagielski wrote:
> > Anyone ever try OpenPA?
> >
> > https://trac.mcs.anl.gov/projects/openpa/
> >
> > It's under MIT, fwiw.
>
> Haven't tried it but the README
>
> http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
>
> indicates only platform support based on gcc plus Windows (but they say
> they haven't set up a build system for it) plus Solaris (but that seems
> to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
> have the needed implementation).
>
> Regards,
>
> Rainer
>
>
> By the way, Rainer, in your l*m*n test combinations, which compiler(s)
> did you use on Solaris while confirming that 2.4.7 Event works at least
> a little?
I'm only building with gcc. On Solaris 8 with gcc 4.1.2, on Solaris 10
the latest and greatest (4.8.2).
Will post the full 2.4.7 results a bit later (only for the sake of
completeness), but the builds are also broken for event on Linux 32 Bits
even when using gcc:
[Thu Nov 21 11:34:53.149765 2013] [mpm_event:crit] [pid 18492:tid
3080439472] AH02405: atomics not working as expected
Sorry for not having checked progress during the tests, but the last
combinations just finished right now.
Regards,
Rainer
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 23.11.2013 14:15, Jeff Trawick wrote:
> On Sat, Nov 23, 2013 at 8:04 AM, Rainer Jung <rainer.jung@kippdata.de
> <ma...@kippdata.de>> wrote:
>
> On 22.11.2013 23:03, Jim Jagielski wrote:
> > Anyone ever try OpenPA?
> >
> > https://trac.mcs.anl.gov/projects/openpa/
> >
> > It's under MIT, fwiw.
>
> Haven't tried it but the README
>
> http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
>
> indicates only platform support based on gcc plus Windows (but they say
> they haven't set up a build system for it) plus Solaris (but that seems
> to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
> have the needed implementation).
>
> Regards,
>
> Rainer
>
>
> By the way, Rainer, in your l*m*n test combinations, which compiler(s)
> did you use on Solaris while confirming that 2.4.7 Event works at least
> a little?
I'm only building with gcc. On Solaris 8 with gcc 4.1.2, on Solaris 10
the latest and greatest (4.8.2).
Will post the full 2.4.7 results a bit later (only for the sake of
completeness), but the builds are also broken for event on Linux 32 Bits
even when using gcc:
[Thu Nov 21 11:34:53.149765 2013] [mpm_event:crit] [pid 18492:tid
3080439472] AH02405: atomics not working as expected
Sorry for not having checked progress during the tests, but the last
combinations just finished right now.
Regards,
Rainer
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Nov 23, 2013 at 8:04 AM, Rainer Jung <ra...@kippdata.de>wrote:
> On 22.11.2013 23:03, Jim Jagielski wrote:
> > Anyone ever try OpenPA?
> >
> > https://trac.mcs.anl.gov/projects/openpa/
> >
> > It's under MIT, fwiw.
>
> Haven't tried it but the README
>
> http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
>
> indicates only platform support based on gcc plus Windows (but they say
> they haven't set up a build system for it) plus Solaris (but that seems
> to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
> have the needed implementation).
>
> Regards,
>
> Rainer
>
>
By the way, Rainer, in your l*m*n test combinations, which compiler(s) did
you use on Solaris while confirming that 2.4.7 Event works at least a
little?
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 22.11.2013 23:03, Jim Jagielski wrote:
> Anyone ever try OpenPA?
>
> https://trac.mcs.anl.gov/projects/openpa/
>
> It's under MIT, fwiw.
Haven't tried it but the README
http://git.mcs.anl.gov/radix/openpa.git/blob_plain/HEAD:/README
indicates only platform support based on gcc plus Windows (but they say
they haven't set up a build system for it) plus Solaris (but that seems
to be Solaris 10+ only, at least my oldest Solaris 8 doesn't seem to
have the needed implementation).
Regards,
Rainer
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Anyone ever try OpenPA?
https://trac.mcs.anl.gov/projects/openpa/
It's under MIT, fwiw.
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Anyone ever try OpenPA?
https://trac.mcs.anl.gov/projects/openpa/
It's under MIT, fwiw.
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 4:27 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>
>>> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>
>>>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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.
>>>
>>
>> It does a decrement long, which sets the zero flag as appropriate. Then
>> it does set-not-equal to set what becomes the return value to 0 if the
>> result of the decrement was 0 and 1 otherwise.
>>
>> It "should be easy" to make it return the new value like the
>> __sync_sub_and_fetch() version does :)
>>
>
> Any Intel assembly gurus out there?
>
> --- apr.orig/atomic/unix/ia32.c 2013-04-21 14:28:47.000000000 -0700
> +++ apr/atomic/unix/ia32.c 2013-11-22 13:16:04.000000000 -0800
> @@ -57,14 +57,14 @@
>
> APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
> {
> - unsigned char prev;
> + apr_uint32_t newvalue;
>
> - asm volatile ("lock; decl %0; setnz %1"
> - : "=m" (*mem), "=qm" (prev)
> + asm volatile ("lock; decl %0; movl %0,%1"
> + : "=m" (*mem), "=qm" (newvalue)
> : "m" (*mem)
> : "memory");
>
> - return prev;
> + return newvalue;
> }
>
> APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
> apr_uint32_t with,
>
> It probably uses a zillion more cycles than the previous version. I
> wonder what __sync_sub_and_fetch() does exactly.
>
> Also, maybe stdcxx has an implementation somewhere that matches
> __sync_sub_and_fetch() (I see
> http://svn.apache.org/repos/asf/stdcxx/branches/4.2.x/include/rw/_atomic-sync.h,
> but no time to look now.
>
"Most" (I won't swear exactly which ones ;) ) APR implementations of
apr_atomic_dec32() return the new value. The z/OS code in
atomic/os390/atomic.c needs an obvious fix too. (IIUC, Event MPM should
work there because of recently added pollset support, though I guess a few
lines in configure and event need to be patched to recognize the new
pollset implementation as usable by event.)
>
>>
>>>
>>> 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 <tr...@gmail.com> wrote:
>>>>>
>>>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>>>> wrote:
>>>>> >
>>>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>>> >
>>>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
>>
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 4:27 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>
>>> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>
>>>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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.
>>>
>>
>> It does a decrement long, which sets the zero flag as appropriate. Then
>> it does set-not-equal to set what becomes the return value to 0 if the
>> result of the decrement was 0 and 1 otherwise.
>>
>> It "should be easy" to make it return the new value like the
>> __sync_sub_and_fetch() version does :)
>>
>
> Any Intel assembly gurus out there?
>
> --- apr.orig/atomic/unix/ia32.c 2013-04-21 14:28:47.000000000 -0700
> +++ apr/atomic/unix/ia32.c 2013-11-22 13:16:04.000000000 -0800
> @@ -57,14 +57,14 @@
>
> APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
> {
> - unsigned char prev;
> + apr_uint32_t newvalue;
>
> - asm volatile ("lock; decl %0; setnz %1"
> - : "=m" (*mem), "=qm" (prev)
> + asm volatile ("lock; decl %0; movl %0,%1"
> + : "=m" (*mem), "=qm" (newvalue)
> : "m" (*mem)
> : "memory");
>
> - return prev;
> + return newvalue;
> }
>
> APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
> apr_uint32_t with,
>
> It probably uses a zillion more cycles than the previous version. I
> wonder what __sync_sub_and_fetch() does exactly.
>
> Also, maybe stdcxx has an implementation somewhere that matches
> __sync_sub_and_fetch() (I see
> http://svn.apache.org/repos/asf/stdcxx/branches/4.2.x/include/rw/_atomic-sync.h,
> but no time to look now.
>
"Most" (I won't swear exactly which ones ;) ) APR implementations of
apr_atomic_dec32() return the new value. The z/OS code in
atomic/os390/atomic.c needs an obvious fix too. (IIUC, Event MPM should
work there because of recently added pollset support, though I guess a few
lines in configure and event need to be patched to recognize the new
pollset implementation as usable by event.)
>
>>
>>>
>>> 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 <tr...@gmail.com> wrote:
>>>>>
>>>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>>>> wrote:
>>>>> >
>>>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>>> >
>>>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
>>
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>
>>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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.
>>
>
> It does a decrement long, which sets the zero flag as appropriate. Then
> it does set-not-equal to set what becomes the return value to 0 if the
> result of the decrement was 0 and 1 otherwise.
>
> It "should be easy" to make it return the new value like the
> __sync_sub_and_fetch() version does :)
>
Any Intel assembly gurus out there?
--- apr.orig/atomic/unix/ia32.c 2013-04-21 14:28:47.000000000 -0700
+++ apr/atomic/unix/ia32.c 2013-11-22 13:16:04.000000000 -0800
@@ -57,14 +57,14 @@
APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
{
- unsigned char prev;
+ apr_uint32_t newvalue;
- asm volatile ("lock; decl %0; setnz %1"
- : "=m" (*mem), "=qm" (prev)
+ asm volatile ("lock; decl %0; movl %0,%1"
+ : "=m" (*mem), "=qm" (newvalue)
: "m" (*mem)
: "memory");
- return prev;
+ return newvalue;
}
APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
It probably uses a zillion more cycles than the previous version. I wonder
what __sync_sub_and_fetch() does exactly.
Also, maybe stdcxx has an implementation somewhere that matches
__sync_sub_and_fetch() (I see
http://svn.apache.org/repos/asf/stdcxx/branches/4.2.x/include/rw/_atomic-sync.h,
but no time to look now.
>
>>
>> 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 <tr...@gmail.com> wrote:
>>>>
>>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>>> wrote:
>>>> >
>>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>> >
>>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>
>>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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.
>>
>
> It does a decrement long, which sets the zero flag as appropriate. Then
> it does set-not-equal to set what becomes the return value to 0 if the
> result of the decrement was 0 and 1 otherwise.
>
> It "should be easy" to make it return the new value like the
> __sync_sub_and_fetch() version does :)
>
Any Intel assembly gurus out there?
--- apr.orig/atomic/unix/ia32.c 2013-04-21 14:28:47.000000000 -0700
+++ apr/atomic/unix/ia32.c 2013-11-22 13:16:04.000000000 -0800
@@ -57,14 +57,14 @@
APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
{
- unsigned char prev;
+ apr_uint32_t newvalue;
- asm volatile ("lock; decl %0; setnz %1"
- : "=m" (*mem), "=qm" (prev)
+ asm volatile ("lock; decl %0; movl %0,%1"
+ : "=m" (*mem), "=qm" (newvalue)
: "m" (*mem)
: "memory");
- return prev;
+ return newvalue;
}
APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
It probably uses a zillion more cycles than the previous version. I wonder
what __sync_sub_and_fetch() does exactly.
Also, maybe stdcxx has an implementation somewhere that matches
__sync_sub_and_fetch() (I see
http://svn.apache.org/repos/asf/stdcxx/branches/4.2.x/include/rw/_atomic-sync.h,
but no time to look now.
>
>>
>> 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 <tr...@gmail.com> wrote:
>>>>
>>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>>> wrote:
>>>> >
>>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>> >
>>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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.
>
It does a decrement long, which sets the zero flag as appropriate. Then it
does set-not-equal to set what becomes the return value to 0 if the result
of the decrement was 0 and 1 otherwise.
It "should be easy" to make it return the new value like the
__sync_sub_and_fetch() version does :)
>
> 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 <tr...@gmail.com> wrote:
>>>
>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>> wrote:
>>> >
>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>> >
>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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.
>
It does a decrement long, which sets the zero flag as appropriate. Then it
does set-not-equal to set what becomes the return value to 0 if the result
of the decrement was 0 and 1 otherwise.
It "should be easy" to make it return the new value like the
__sync_sub_and_fetch() version does :)
>
> 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 <tr...@gmail.com> wrote:
>>>
>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com>
>>> wrote:
>>> >
>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>> >
>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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 <tr...@gmail.com> wrote:
>>
>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> >
>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>> >
>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 22, 2013, at 3:24 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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...
>
Yeah.
> 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; }"
>
>
>
> You should be seeing it in trunk as well...
>
>
> On Nov 22, 2013, at 2:43 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> >
> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
> >
> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <ji...@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; }"
>
> You should be seeing it in trunk as well...
>
>
> On Nov 22, 2013, at 2:43 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> >
> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
> >
> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
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...
You should be seeing it in trunk as well...
On Nov 22, 2013, at 2:43 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
> On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
> > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
> On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
> > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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/
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 22, 2013, at 2:22 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@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?
Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> jim@apache.org wrote:
> > Author: jim
> > Date: Fri Nov 16 16:49:31 2012
> > New Revision: 1410459
> >
> > URL: http://svn.apache.org/viewvc?rev=1410459&view=rev
> > Log:
> > fdq expects a certain behavior from atomics... ensure that
> > the event mpms check this.
> >
> > Modified:
> > httpd/httpd/trunk/docs/log-message-tags/next-number
> > httpd/httpd/trunk/server/mpm/event/event.c
> > httpd/httpd/trunk/server/mpm/eventopt/eventopt.c
> >
>
> > Modified: httpd/httpd/trunk/server/mpm/event/event.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1410459&r1=1410458&r2=1410459&view=diff
> >
> ==============================================================================
> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Nov 16 16:49:31 2012
> > @@ -2928,6 +2928,18 @@ static int event_pre_config(apr_pool_t *
> > }
> > ++retained->module_loads;
> > if (retained->module_loads == 2) {
> > + int i;
> > + static apr_uint32_t foo = 0;
> > +
> > + apr_atomic_inc32(&foo);
> > + apr_atomic_dec32(&foo);
> > + apr_atomic_dec32(&foo);
> > + 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.
> > + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
> APLOGNO(02405)
> > + "atomics not working as expected");
> > + return HTTP_INTERNAL_SERVER_ERROR;
> > + }
> > rv = apr_pollset_create(&event_pollset, 1, plog,
> > APR_POLLSET_THREADSAFE |
> APR_POLLSET_NOCOPY);
> > if (rv != APR_SUCCESS) {
> >
> >
>
> Regards
>
> Rüdiger
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/