apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r1410459 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/mpm/event/event.c server/mpm/eventopt/eventopt.c
Date Fri, 22 Nov 2013 22:47:55 GMT
On Fri, Nov 22, 2013 at 4:27 PM, Jeff Trawick <trawick@gmail.com> wrote:

> On Fri, Nov 22, 2013 at 3:57 PM, Jeff Trawick <trawick@gmail.com> wrote:
>
>> On Fri, Nov 22, 2013 at 3:40 PM, Jeff Trawick <trawick@gmail.com> wrote:
>>
>>> On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <trawick@gmail.com> wrote:
>>>
>>>> On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski <jim@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 <trawick@gmail.com> wrote:
>>>>>
>>>>> > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski <jim@jagunet.com>
>>>>> wrote:
>>>>> >
>>>>> > On Nov 22, 2013, at 2:22 PM, Jeff Trawick <trawick@gmail.com>
wrote:
>>>>> >
>>>>> > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem <rpluem@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/

Mime
View raw message