On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick <traw...@gmail.com> wrote:
> 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 :) > 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 <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/ > -- Born in Roswell... married an alien... http://emptyhammock.com/
