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 5D240108D0 for ; Fri, 22 Nov 2013 20:41:17 +0000 (UTC) Received: (qmail 82903 invoked by uid 500); 22 Nov 2013 20:41:16 -0000 Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 82853 invoked by uid 500); 22 Nov 2013 20:41:16 -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 82845 invoked by uid 99); 22 Nov 2013 20:41:16 -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 20:41:16 +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.177 as permitted sender) Received: from [209.85.217.177] (HELO mail-lb0-f177.google.com) (209.85.217.177) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 22 Nov 2013 20:41:10 +0000 Received: by mail-lb0-f177.google.com with SMTP id w7so1384169lbi.8 for ; Fri, 22 Nov 2013 12:40:50 -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=7hrTRrfI0CB6I4shmKIMfJeqJ7i0zpABiQ2ViCsFq2g=; b=iCtUX36XnENBKBe030u/LQwvTE8I4Vsjn3jujtMXKPGsEoSeTpcT+Rnbehd4tGEiv4 sov1DWPrDm8X0ANRRKGodXds6q+MVRW0ESasGy52nBLiSZLPDKu/s/eliTN3jTMS3K/B KYYs407dY94j6F/hn4ngu3vhoz5SLgkx1K3dACfDTV5zl8y+wJtlaIhjfigu8PqK5d2X 17ziLstHtbprGNNdNOLBQh5Ts50wdVCH9ZelNwn7SvFVpifl5Towx29kT86fqvSd/iqR Hr8rbmV37wN6gzkO0iNAwsBphij/8Mae125wq3KcKrurS048tlgjOXguEG6EnzWqtVTj X3bA== MIME-Version: 1.0 X-Received: by 10.152.6.201 with SMTP id d9mr7899948laa.25.1385152850135; Fri, 22 Nov 2013 12:40:50 -0800 (PST) Received: by 10.114.183.226 with HTTP; Fri, 22 Nov 2013 12:40:50 -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 15:40:50 -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 Content-Type: multipart/alternative; boundary=089e013d1a94978ece04ebca0791 X-Virus-Checked: Checked by ClamAV on apache.org --089e013d1a94978ece04ebca0791 Content-Type: text/plain; charset=ISO-8859-1 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/ --089e013d1a94978ece04ebca0791 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Fri, Nov 22, 2013 at 3:24 PM, Jeff Trawick <trawick@gm= ail.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=3D1= 542560)
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...

<= div>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() { un= signed char x; =A0... return x; =A0}"


APR is documented as returning "zero if the value becomes zero on d= ecrement, otherwise non-zero".

With gcc we us= e get __sync_sub_and_fetch(), which returns the new value after the decreme= nt.

With icc we use the assembler implementation in APR. = =A0I 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)
{
=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_ato= mic_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 "prev_idlers" here is deceiving.)

=A0

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:<= br> > >
> >
> > jim@apache.or= g wrote:
> > > + =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 the dec= causes foo to become zero and it returns non zero
> > otherwise. Shouldn't this behavior the same across all platfo= rms? 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 expect= ed or else
> =A0 =A0 =A0* =A0 =A0 =A0 rework how we determine blocked.
> =A0 =A0 =A0* UPDATE: Correct operation is performed during open_logs()=
> =A0 =A0 =A0*/
> =A0 =A0 prev_idlers =3D apr_atomic_inc32((apr_uint32_t *)&(queue_i= nfo->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...") = for
> the reason, etc.
>
> When you say "icc (Intel) builds of httpd 2.4.7 event MPM (with a= pr-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://emptyham= mock.com/




--
Born in Roswell... married an alien...http://emptyhammoc= k.com/



--
Born in Rosw= ell... married an alien...
http://emptyhammock.com/
--089e013d1a94978ece04ebca0791--