On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <traw...@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <traw...@gmail.com> wrote: > >> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <j...@jagunet.com> wrote: >> >>> Note, the only think changed in event now (via >>> https://svn.apache.org/viewvc?view=revision&revision=1542560) >>> is that event *checks* that atomics work as required for >>> event... if the check fails, it means that event has >>> been "broken" on that system, assuming it ever hit >>> blocked idlers, for a *long* time... >>> >> >> Got it... fdqueue.c is asking for trouble... >> >> I'm using atomic/unix/ia32.c with icc too. >> >> Need to compare generated code... I hate stuff like "int foo() { >> unsigned char x; ... return x; }" >> >> > APR is documented as returning "zero if the value becomes zero on > decrement, otherwise non-zero". > > With gcc we use get __sync_sub_and_fetch(), which returns the new value > after the decrement. > > With icc we use the assembler implementation in APR. I think that's > returning 1 instead of -1. > 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 <traw...@gmail.com> wrote: >>> >>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <j...@jagunet.com> >>> wrote: >>> > >>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <traw...@gmail.com> wrote: >>> > >>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rpl...@apache.org> >>> wrote: >>> > > >>> > > >>> > > j...@apache.org wrote: >>> > > > + i = apr_atomic_dec32(&foo); >>> > > > + if (i >= 0) { >>> > > >>> > > Why can we expect i < 0? apr_atomic_dec32 returns 0 if the dec >>> causes foo to become zero and it returns non zero >>> > > otherwise. Shouldn't this behavior the same across all platforms? >>> And if not should that be fixed in APR? >>> > > >>> > > icc (Intel) builds of httpd 2.4.7 event MPM (with apr-1.5.0) bomb >>> here. >>> > > >>> > > --enable-nonportable-atomics is specified for apr, though I haven't >>> checked what that does with icc. >>> > > >>> > >>> > As noted back with the orig update, this test is due to the >>> > fdqueue code in the new event: >>> > >>> > apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info, >>> > apr_pool_t * pool_to_recycle) >>> > { >>> > apr_status_t rv; >>> > int prev_idlers; >>> > >>> > ap_push_pool(queue_info, pool_to_recycle); >>> > >>> > /* Atomically increment the count of idle workers */ >>> > /* >>> > * TODO: The atomics expect unsigned whereas we're using signed. >>> > * Need to double check that they work as expected or else >>> > * rework how we determine blocked. >>> > * UPDATE: Correct operation is performed during open_logs() >>> > */ >>> > prev_idlers = apr_atomic_inc32((apr_uint32_t >>> *)&(queue_info->idlers)); >>> > >>> > /* If other threads are waiting on a worker, wake one up */ >>> > if (prev_idlers < 0) { >>> > >>> > >>> > See the comments ("The atomics expect unsigned whereas...") for >>> > the reason, etc. >>> > >>> > When you say "icc (Intel) builds of httpd 2.4.7 event MPM (with >>> apr-1.5.0) bomb here." >>> > do you mean that you get the 'atomics not working as expected' error >>> > (and the internal server error) or that it core dumps? >>> > >>> > >>> > "atomics not working as expected" >>> > >>> > Let me see what code is used... >>> > >>> > -- >>> > Born in Roswell... married an alien... >>> > http://emptyhammock.com/ >>> >>> >> >> >> -- >> Born in Roswell... married an alien... >> http://emptyhammock.com/ >> > > > > -- > Born in Roswell... married an alien... > http://emptyhammock.com/ > -- Born in Roswell... married an alien... http://emptyhammock.com/
