Return-Path: X-Original-To: apmail-httpd-dev-archive@www.apache.org Delivered-To: apmail-httpd-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 98E0E10DFD for ; Fri, 22 Nov 2013 23:22:30 +0000 (UTC) Received: (qmail 63821 invoked by uid 500); 22 Nov 2013 23:22:29 -0000 Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 63657 invoked by uid 500); 22 Nov 2013 23:22:29 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 63640 invoked by uid 99); 22 Nov 2013 23:22:29 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 22 Nov 2013 23:22:29 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of trawick@gmail.com designates 209.85.217.169 as permitted sender) Received: from [209.85.217.169] (HELO mail-lb0-f169.google.com) (209.85.217.169) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 22 Nov 2013 23:22:22 +0000 Received: by mail-lb0-f169.google.com with SMTP id y6so1601917lbh.28 for ; Fri, 22 Nov 2013 15:22:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=0ntH8aT4rH2iYvV/QhbAxQ5rCznlsnjnDu7AUjtUCCM=; b=hcbjH/CcUEJCPhUCbXJM98jcSOo/aJ03g+6wfoJY5aFi3FqjPhBbq34mvZKauFz9N6 qFMNm8XerXY7OIC3XkfpYVD2juAkvuM9ZZNupNPVMwJSnA7tCoeJALDb4EJdtDOn5Peh xvdbsei4zILugTA/GaQvMyJAfyuDUj8p6+9JiA4JDnkA79sk5v5opzqi+yBE8kX7xe88 DCBVo971Jzbm6s2SmI31GZX726PZNzk9Nh46qJh1im9IGZwED2DclJCHCOQaBNT/y6rD 3TZsr2h9q5r9MXXKHq2oRVf9GgetPWOTW0vfOQRGYcG276hUFWkm2FMX/dNajunudo37 JHuQ== MIME-Version: 1.0 X-Received: by 10.152.216.167 with SMTP id or7mr11629013lac.10.1385162522251; Fri, 22 Nov 2013 15:22:02 -0800 (PST) Received: by 10.114.183.226 with HTTP; Fri, 22 Nov 2013 15:22:02 -0800 (PST) In-Reply-To: References: <20121116164934.0B98423889D7@eris.apache.org> <50A76E35.2030409@apache.org> <6087E795-DBDA-423E-B5BF-C98FEC28A6AF@jaguNET.com> Date: Fri, 22 Nov 2013 18:22:02 -0500 Message-ID: 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 From: Jeff Trawick To: Apache HTTP Server Development List , APR Developer List Content-Type: multipart/alternative; boundary=001a1135e31818551104ebcc48d5 X-Virus-Checked: Checked by ClamAV on apache.org --001a1135e31818551104ebcc48d5 Content-Type: text/plain; charset=ISO-8859-1 On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski wrote: > The thing is is not only do we worry about the return code > but also that the values we dec32 and inc32 also behave > as signed ints. Note below we worry also that queue_info->idlers > itself is signed, and can be < 0 : > Okay gurus, tell me where I'm confused (maybe I'm trainable): I guess the opportunity for failure is if one of the APR implementations of apr_atomic_dec32() is setting the return value in C code. But doing that (i.e., not having the assembly code set a local variable which is returned) is uncommon. Imagine that this existing z/OS code is changed to simply "return new_val". Conversion from 32-bit unsigned int to 32-bit signed int is undefined IIUC and in the unlikely case that the compiler tried to change the bits some trick would be needed in the code. int apr_atomic_dec32(volatile apr_uint32_t *mem) { apr_uint32_t old, new_val; old = *mem; /* old is automatically updated on cs failure */ do { new_val = old - 1; } while (__cs(&old, (cs_t *)mem, new_val)); return new_val != 0; } > 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; > } > > apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info, > int *had_to_block) > { > apr_status_t rv; > int prev_idlers; > > /* Atomically decrement the idle worker count, saving the old value */ > /* See TODO in ap_queue_info_set_idle() */ > prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), > -1); > > /* Block if there weren't any idle workers */ > if (prev_idlers <= 0) { > rv = apr_thread_mutex_lock(queue_info->idlers_mutex); > if (rv != APR_SUCCESS) { > AP_DEBUG_ASSERT(0); > /* See TODO in ap_queue_info_set_idle() */ > apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers)); /* > back out dec */ > return rv; > } > /* Re-check the idle worker count to guard against a > * race condition. Now that we're in the mutex-protected > * region, one of two things may have happened: > * - If the idle worker count is still negative, the > * workers are all still busy, so it's safe to > * block on a condition variable. > * - If the idle worker count is non-negative, then a > * worker has become idle since the first check > * of queue_info->idlers above. It's possible > * that the worker has also signaled the condition > * variable--and if so, the listener missed it > * because it wasn't yet blocked on the condition > * variable. But if the idle worker count is > * now non-negative, it's safe for this function to > * return immediately. > * > * A negative value in queue_info->idlers tells how many > * threads are waiting on an idle worker. > */ > if (queue_info->idlers < 0) { > *had_to_block = 1; > rv = apr_thread_cond_wait(queue_info->wait_for_idler, > queue_info->idlers_mutex); > > On Nov 22, 2013, at 3:40 PM, Jeff Trawick wrote: > > > On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick wrote: > > On Fri, Nov 22, 2013 at 2:52 PM, Jim Jagielski 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 wrote: > > > > > On Fri, Nov 22, 2013 at 2:39 PM, Jim Jagielski > wrote: > > > > > > On Nov 22, 2013, at 2:22 PM, Jeff Trawick wrote: > > > > > > > On Sat, Nov 17, 2012 at 6:00 AM, Ruediger Pluem > 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/ --001a1135e31818551104ebcc48d5 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Fri, Nov 22, 2013 at 4:32 PM, Jim Jagielski <jim@jagun= et.com> wrote:
The thing is is not only do we worry about the return code=
but also that the values we dec32 and inc32 also behave
as signed ints. Note below we worry also that queue_info->idlers
itself is signed, and can be < 0 :

O= kay gurus, tell me where I'm confused (maybe I'm trainable):
<= div>
I guess the opportunity for failure is if one of the APR= implementations of apr_atomic_dec32() is setting the return value in C cod= e. =A0But doing that (i.e., not having the assembly code set a local variab= le which is returned) is uncommon.

Imagine that this existing z/OS code is changed to simp= ly "return new_val". =A0Conversion from 32-bit unsigned int to 32= -bit signed int is undefined IIUC and in the unlikely case that the compile= r tried to change the bits some trick would be needed in the code.

int apr_atomic_dec32(volatile apr_uint32_t *mem)
{
=A0 =A0 apr_uint32_t old, new_val;=A0

=A0 =A0 old =3D *mem; =A0 /* old is automatically updated on cs fa= ilure */
=A0 =A0 do {
=A0 =A0 =A0 =A0 new_val =3D old - 1;
= =A0 =A0 } while (__cs(&old, (cs_t *)mem, new_val));=A0

=A0 =A0 return new_val !=3D 0;
}



apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)
{
=A0 =A0 int prev_idlers;
=A0 =A0 prev_idlers =3D apr_atomic_dec32((apr_uint32_t *)&(queue_info-&= gt;idlers));
=A0 =A0 if (prev_idlers <=3D 0) {
=A0 =A0 =A0 =A0 apr_atomic_inc32((apr_uint32_t *)&(queue_info->idler= s)); =A0 =A0/* back out dec */
=A0 =A0 =A0 =A0 return APR_EAGAIN;
=A0 =A0 }
=A0 =A0 return APR_SUCCESS;
}

apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_inf= o,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 int *had_to_block)
{
=A0 =A0 apr_status_t rv;
=A0 =A0 int prev_idlers;

=A0 =A0 /* Atomically decrement the idle worker count, saving the old value= */
=A0 =A0 /* See TODO in ap_queue_info_set_idle() */
=A0 =A0 prev_idlers =3D apr_atomic_add32((apr_uint32_t *)&(queue_info-&= gt;idlers), -1);

=A0 =A0 /* Block if there weren't any idle workers */
=A0 =A0 if (prev_idlers <=3D 0) {
=A0 =A0 =A0 =A0 rv =3D apr_thread_mutex_lock(queue_info->idlers_mutex);<= br> =A0 =A0 =A0 =A0 if (rv !=3D APR_SUCCESS) {
=A0 =A0 =A0 =A0 =A0 =A0 AP_DEBUG_ASSERT(0);
=A0 =A0 =A0 =A0 =A0 =A0 /* See TODO in ap_queue_info_set_idle() */
=A0 =A0 =A0 =A0 =A0 =A0 apr_atomic_inc32((apr_uint32_t *)= &(queue_info->idlers)); =A0 =A0/* back out dec */
=A0 =A0 =A0 =A0 =A0 =A0 return rv;
=A0 =A0 =A0 =A0 }
=A0 =A0 =A0 =A0 /* Re-check the idle worker count to guard against a
=A0 =A0 =A0 =A0 =A0* race condition. =A0Now that we're in the mutex-pro= tected
=A0 =A0 =A0 =A0 =A0* region, one of two things may have happened:
=A0 =A0 =A0 =A0 =A0* =A0 - If the idle worker count is still negative, the<= br> =A0 =A0 =A0 =A0 =A0* =A0 =A0 workers are all still busy, so it's safe t= o
=A0 =A0 =A0 =A0 =A0* =A0 =A0 block on a condition variable.
=A0 =A0 =A0 =A0 =A0* =A0 - If the idle worker count is non-negative, then a=
=A0 =A0 =A0 =A0 =A0* =A0 =A0 worker has become idle since the first check =A0 =A0 =A0 =A0 =A0* =A0 =A0 of queue_info->idlers above. =A0It's po= ssible
=A0 =A0 =A0 =A0 =A0* =A0 =A0 that the worker has also signaled the conditio= n
=A0 =A0 =A0 =A0 =A0* =A0 =A0 variable--and if so, the listener missed it =A0 =A0 =A0 =A0 =A0* =A0 =A0 because it wasn't yet blocked on the condi= tion
=A0 =A0 =A0 =A0 =A0* =A0 =A0 variable. =A0But if the idle worker count is =A0 =A0 =A0 =A0 =A0* =A0 =A0 now non-negative, it's safe for this funct= ion to
=A0 =A0 =A0 =A0 =A0* =A0 =A0 return immediately.
=A0 =A0 =A0 =A0 =A0*
=A0 =A0 =A0 =A0 =A0* =A0 =A0 A negative value in queue_info->idlers tell= s how many
=A0 =A0 =A0 =A0 =A0* =A0 =A0 threads are waiting on an idle worker.
=A0 =A0 =A0 =A0 =A0*/
=A0 =A0 =A0 =A0 if (queue_info->idlers < 0) {
=A0 =A0 =A0 =A0 =A0 =A0 *had_to_block =3D 1;
=A0 =A0 =A0 =A0 =A0 =A0 rv =3D apr_thread_cond_wait(queue_info->wait_for= _idler,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= queue_info->idlers_mutex);

On 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=3Drevision&revision=3D1542560)<= br> > 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... =A0fdqueue.c is asking for trouble...
>
> I'm using atomic/unix/ia32.c with icc too.
>
> Need to compare generated code... =A0I hate stuff like "int foo()= { unsigned char x; =A0... return x; =A0}"
>
>
> 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 valu= e after the decrement.
>
> With icc we use the assembler implementation in APR. =A0I think that&#= 39;s returning 1 instead of -1.
>
> Here is where fdqueue needs to be able to see a negative return code:<= br> >
> apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info)=
> {
> =A0 =A0 int prev_idlers;
> =A0 =A0 prev_idlers =3D apr_atomic_dec32((apr_uint32_t *)&(queue_i= nfo->idlers));
> =A0 =A0 if (prev_idlers <=3D 0) {
> =A0 =A0 =A0 =A0 apr_atomic_inc32((apr_uint32_t *)&(queue_info->= idlers)); =A0 =A0/* back out dec */
> =A0 =A0 =A0 =A0 return APR_EAGAIN;
> =A0 =A0 }
> =A0 =A0 return APR_SUCCESS;
> }
>
> ("prev" in the ia32.c version of apr_atomic_dec32() and &quo= t;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:<= br> > > > > + =A0 =A0 =A0 =A0i =3D apr_atomic_dec32(&foo);
> > > > + =A0 =A0 =A0 =A0if (i >=3D 0) {
> > >
> > > Why can we expect i < 0? apr_atomic_dec32 returns 0 if th= e dec causes foo to become zero and it returns non zero
> > > otherwise. Shouldn't this behavior the same across all p= latforms? 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,=
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 apr_pool_t * pool_to_recycle)
> > {
> > =A0 =A0 apr_status_t rv;
> > =A0 =A0 int prev_idlers;
> >
> > =A0 =A0 ap_push_pool(queue_info, pool_to_recycle);
> >
> > =A0 =A0 /* Atomically increment the count of idle workers */
> > =A0 =A0 /*
> > =A0 =A0 =A0* TODO: The atomics expect unsigned whereas we're = using signed.
> > =A0 =A0 =A0* =A0 =A0 =A0 Need to double check that they work as e= xpected or else
> > =A0 =A0 =A0* =A0 =A0 =A0 rework how we determine blocked.
> > =A0 =A0 =A0* UPDATE: Correct operation is performed during open_l= ogs()
> > =A0 =A0 =A0*/
> > =A0 =A0 prev_idlers =3D apr_atomic_inc32((apr_uint32_t *)&(qu= eue_info->idlers));
> >
> > =A0 =A0 /* If other threads are waiting on a worker, wake one up = */
> > =A0 =A0 if (prev_idlers < 0) {
> >
> >
> > See the comments ("The atomics expect unsigned whereas...&qu= ot;) for
> > the reason, etc.
> >
> > When you say "icc (Intel) builds of httpd 2.4.7 event MPM (w= ith 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://emp= tyhammock.com/
>
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyham= mock.com/
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyham= mock.com/




--
= Born in Roswell... married an alien...
http://emptyhammock.com/
--001a1135e31818551104ebcc48d5--