httpd-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 Mon, 25 Nov 2013 00:18:05 GMT
On Sat, Nov 23, 2013 at 5:39 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> Couldn't ap_queue_info_try_get_idler() and the event_pre_config() check
> use :
>
>
>     prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers),
> -1);
>
> like ap_queue_info_wait_for_idler() does ?
>

I think that's correct...

What the code really wants is new_idlers, so edit that slightly to

new_idlers =  apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), -1) -
1;
if (new_idlers <= 0) {
    ...
    return APR_EAGAIN;
}



>
> Or maybe queue_info->idlers be declared uint32_t  and negatives computed
> relative to 2^31 ?
>

The comments say that the uint/int discrepancy is the problem, but the
actual problem is the expectation that apr_atomic_dec32() returns the new
value, when all that is promised is a zero/non-zero flag.  The new value is
what is returned in almost all implementations, but that is not actually
promised by the API, and promising new value (perhaps in apr-future) is
rather ugly with a declared return type of int.


> Regards,
> Yann.
>
>
>
>
> On Fri, Nov 22, 2013 at 9: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.
>>
>> 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