You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by wr...@apache.org on 2008/09/26 22:15:10 UTC
svn commit: r699481 - /httpd/httpd/trunk/server/mpm/winnt/child.c
Author: wrowe
Date: Fri Sep 26 13:15:10 2008
New Revision: 699481
URL: http://svn.apache.org/viewvc?rev=699481&view=rev
Log:
Reimplement ThreadStackSize to behave as on unix for any
Windows 2003/2008 (XP/Vista) servers. Virtual allocations
will only consume pages once referenced, while the page
alignment will vary by ThreadStackSize setting so that the
maximum number of threads and minimum stack VM profile will
be wasted.
Modified:
httpd/httpd/trunk/server/mpm/winnt/child.c
Modified: httpd/httpd/trunk/server/mpm/winnt/child.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/winnt/child.c?rev=699481&r1=699480&r2=699481&view=diff
==============================================================================
--- httpd/httpd/trunk/server/mpm/winnt/child.c (original)
+++ httpd/httpd/trunk/server/mpm/winnt/child.c Fri Sep 26 13:15:10 2008
@@ -984,13 +984,14 @@
continue;
}
ap_update_child_status_from_indexes(0, i, SERVER_STARTING, NULL);
- child_handles[i] = (HANDLE) _beginthreadex(
- NULL, (unsigned)ap_thread_stacksize,
- worker_main, (void *) i, 0, &tid);
+
+ child_handles[i] = CreateThread(NULL, ap_thread_stacksize,
+ worker_main, (void *) i,
+ stack_res_flag, &tid);
if (child_handles[i] == 0) {
ap_log_error(APLOG_MARK, APLOG_CRIT, apr_get_os_error(),
ap_server_conf,
- "Child %d: _beginthreadex failed. Unable to "
+ "Child %d: CreateThread failed. Unable to "
"create all worker threads. Created %d of the %d "
"threads requested with the ThreadsPerChild "
"configuration directive.",
Re: svn commit: r699481 - /httpd/httpd/trunk/server/mpm/winnt/child.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
William A. Rowe, Jr. wrote:
> Mladen Turk wrote:
>> wrowe@apache.org wrote:
>>> Author: wrowe
>>> Date: Fri Sep 26 13:15:10 2008
>>> New Revision: 699481
>>>
>>> URL: http://svn.apache.org/viewvc?rev=699481&view=rev
>>> Log:
>>> Reimplement ThreadStackSize to behave as on unix for any
>>> Windows 2003/2008 (XP/Vista) servers. Virtual allocations
>>> will only consume pages once referenced, while the page
>>> alignment will vary by ThreadStackSize setting so that the
>>> maximum number of threads and minimum stack VM profile will
>>> be wasted.
>>>
>>> - child_handles[i] = (HANDLE) _beginthreadex(
>>> + child_handles[i] = CreateThread(NULL, ap_thread_stacksize,
>> Why did you change the _beginthreadex to CreateThread?
>> Won't that bring back CRT local Tls problems that forced using
>> _beginthreadex at the first place?
>
> Please site the Tls problems. There is one I can think of, which is
> the behavior of the locale (inherit from parent v.s. inherit from global)
> but that's not even a portable contract between unix and windows modules.
> Otherwise, TLS is allocated on first-reference.
OK... here we go, sorry it wasn't on a convenient machine when I posted this
message in the first place;
http://blogs.msdn.com/michkap/archive/2008/01/02/6950506.aspx
It appears that only if you started playing with _ENABLE_PER_THREAD_LOCALE
(something unwise for httpd or even apr, since it's far from portable) will
you observe some interesting side effects.
> _beginthreadex() is further dangerous because returning from that thread,
> before return, the handle is *closed*. Elsewhere we are testing those
> handles to see if the thread has terminated, potentially what it's return
> code is. So closing the handle before we do is invalid behavior.
This was my reason. In the mpm (not in apr) I deliberately choose CreateThread
for every handle we utilize and close beyond the lifespan of the thread, while
I left _beginthreadex for those threads which are simply started and forgotten.
The _beginthreadex pragma will do the thread handle cleanup for us on end thread.
>> Also I have concerns with chosen 64K thread stack size for
>> some other threads in this patch series. Perhaps a 256K will
>> be more safe (particularly for winnt_accept), because 64K on
>> stack overflow OS will allocate 1M actually, so this either needs to
>> get carefully calculated or traced by some tool, cause larger
>> initial commit size might eventually use less memory.
>
> Are you sure? Please validate your assumptions with actual testing
> before postulating; the original behavior and assumptions of the entire
> ThreadStackSize behavior on windows was so off base as to be laughable.
> These changes are all based on direct observation, on Windows XP (2003)
> to be confirmed on 2008 before I propose for backport.
AFAICT, there's one chance. An overflow can't extend the stack when there
is another stack right afterwards, so on overflow httpd can and should fault.
Note that with the flag changes, these are true virtual allocation sizes,
no longer preallocation within the oversized thread spaces. That's the unix
behavior, so that should be the win32 behavior. I've managed to crank up to
the 15,000 threadlimit hard limit.
Why, for that matter, do we have a hardcoded ThreadLimit cap? Since we have
a default ThreadLimit cap, the hardcode cap seems silly, as ThreadLimit now
controls storage and reservations, and is tuned as needed by the user.
This seems like now-useless, legacy cruft.
> Can you look at those threads and please justify a larger stack size
> rather than just kicking them up for the sake of it? Based on rev 88498
> I'm pretty sure that several choices were made without any real basis
> to get the code adjusted, and they never were revisited.
I have one major test before this is allowed to remain. The stacks I checked
out and my initial review was all based on 32 bit win32. On 64 bit, obviously
that stack expands in size due to pointer sizes and alignment.
Re: svn commit: r699481 - /httpd/httpd/trunk/server/mpm/winnt/child.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Mladen Turk wrote:
> wrowe@apache.org wrote:
>> Author: wrowe
>> Date: Fri Sep 26 13:15:10 2008
>> New Revision: 699481
>>
>> URL: http://svn.apache.org/viewvc?rev=699481&view=rev
>> Log:
>> Reimplement ThreadStackSize to behave as on unix for any
>> Windows 2003/2008 (XP/Vista) servers. Virtual allocations
>> will only consume pages once referenced, while the page
>> alignment will vary by ThreadStackSize setting so that the
>> maximum number of threads and minimum stack VM profile will
>> be wasted.
>>
>> - child_handles[i] = (HANDLE) _beginthreadex(
>
>> + child_handles[i] = CreateThread(NULL, ap_thread_stacksize,
>
> Why did you change the _beginthreadex to CreateThread?
> Won't that bring back CRT local Tls problems that forced using
> _beginthreadex at the first place?
Please site the Tls problems. There is one I can think of, which is
the behavior of the locale (inherit from parent v.s. inherit from global)
but that's not even a portable contract between unix and windows modules.
Otherwise, TLS is allocated on first-reference.
_beginthreadex() is further dangerous because returning from that thread,
before return, the handle is *closed*. Elsewhere we are testing those
handles to see if the thread has terminated, potentially what it's return
code is. So closing the handle before we do is invalid behavior.
> Also I have concerns with chosen 64K thread stack size for
> some other threads in this patch series. Perhaps a 256K will
> be more safe (particularly for winnt_accept), because 64K on
> stack overflow OS will allocate 1M actually, so this either needs to
> get carefully calculated or traced by some tool, cause larger
> initial commit size might eventually use less memory.
Are you sure? Please validate your assumptions with actual testing
before postulating; the original behavior and assumptions of the entire
ThreadStackSize behavior on windows was so off base as to be laughable.
These changes are all based on direct observation, on Windows XP (2003)
to be confirmed on 2008 before I propose for backport.
Can you look at those threads and please justify a larger stack size
rather than just kicking them up for the sake of it? Based on rev 88498
I'm pretty sure that several choices were made without any real basis
to get the code adjusted, and they never were revisited.
Re: svn commit: r699481 - /httpd/httpd/trunk/server/mpm/winnt/child.c
Posted by Mladen Turk <mt...@apache.org>.
wrowe@apache.org wrote:
> Author: wrowe
> Date: Fri Sep 26 13:15:10 2008
> New Revision: 699481
>
> URL: http://svn.apache.org/viewvc?rev=699481&view=rev
> Log:
> Reimplement ThreadStackSize to behave as on unix for any
> Windows 2003/2008 (XP/Vista) servers. Virtual allocations
> will only consume pages once referenced, while the page
> alignment will vary by ThreadStackSize setting so that the
> maximum number of threads and minimum stack VM profile will
> be wasted.
>
> - child_handles[i] = (HANDLE) _beginthreadex(
> + child_handles[i] = CreateThread(NULL, ap_thread_stacksize,
Why did you change the _beginthreadex to CreateThread?
Won't that bring back CRT local Tls problems that forced using
_beginthreadex at the first place?
Also I have concerns with chosen 64K thread stack size for
some other threads in this patch series. Perhaps a 256K will
be more safe (particularly for winnt_accept), because 64K on
stack overflow OS will allocate 1M actually, so this either needs to
get carefully calculated or traced by some tool, cause larger
initial commit size might eventually use less memory.
Regards
--
^(TM)