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