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 20:57:20 GMT
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 :)


>
> 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/

Mime
View raw message