Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-21131: don't use SIGSTKSZ for stack size in sigaltstack #13649

Open
wants to merge 5 commits into
base: master
from

Conversation

@peadar
Copy link

commented May 29, 2019

Following up discussion on http://bugs.python.org/issue21131

vstinner suggested using the logic in thread_pthread.h to work out the "good" stack size for sigaltstack. I've integrated the logic in there, but it doesn't actually provide an accessible way of finding the default thread stack size - it just nominally requests pthread_create to create a stack of a "default" size by not poking at the stack size attribute.

So, instead, I've rejigged the macros in that file to provide PLATFORM_STACK_SIZE and PLATFORM_STACK_PREFERRED that are available regardless of the ability of pthreads to set the thread stack size attribute (i.e., regardless of the presence of _POSIX_THREAD_ATTR_STACK_SIZE) (and simplified some of the conditionals in the code using THREAD_STACK_SIZE in the process). The eventual sizes past to pthread_attr_setstacksize are unaffected.

This is then used as a fallback in the new function, size_t _Pthread_preferred_stacksize() , which will first attempt to use a default-constructed pthread_attr_t to work out at runtime what size stack the system prefers for new threads. This value is far more likely to be usable than the value defined in SIGSTKSZ.

If that call fails, or the thread size attribute is not available, we default to the maximum of PTHREAD_STACK_MIN, whatever we've defined for PLATFORM_STACK_SIZE for those platforms with an inadequate default stacksize, or finally the arbitrary 32k setting previously used as a floor.

All feedback gratefully appreciated.

https://bugs.python.org/issue21131

@peadar peadar force-pushed the peadar:issue21131 branch from c169585 to 6ee50df May 29, 2019

@csabella csabella requested a review from vstinner May 29, 2019

@auvipy
Copy link

left a comment

news entry needed if it is not private.

@peadar peadar force-pushed the peadar:issue21131 branch from 3f945eb to 938c0df May 31, 2019

@peadar peadar force-pushed the peadar:issue21131 branch from 938c0df to 73e88fe Jul 8, 2019

@justbennet

This comment has been minimized.

Copy link

commented Aug 9, 2019

Is there any chance that the priority of this could get promoted prior to the next release? This causes the FaultHandler test to segfault when compiling on the Skylake architecture. This seems somewhat serious, yet it's not been reviewed, and I think perhaps is should not be a type-enhancement but an actual bug?

Adding @ambv here in lieu of sending mail per the release candidate notes at
https://www.python.org/downloads/release/python-380b3/

peadar added some commits May 29, 2019

bpo-21131: Add _Pythread_preferred_stacksize
This function gives a value, in bytes, that represents a usable amount
of memory to use for a stack on the host system.

The existing thread module does provide THREAD_STACKSIZE, but this is
only set if the pthreads implementation supports changing thread stack
sizes, and is nominally set to 0, to indicate that pthreads can choose
the stack size of new threads.

We need to be able to create stacks for other reasons, however, and
it is useful to be able to query the system for it's preferred stack
size. (specifically for faulthandler and sigaltstack())

We also provide an implementation for pthread-nt.h, that dumbly returns
1MB. This will not currently be used, as sigaltstack is not being used
for faulthandler on Windows. This is apparently the default windows stack
size as inserted into the executable header by the linker. I don't know
enough win32 API to work out how to extract the value for the current exe.
bpo-21131: Use _Pythread_preferred_stacksize in sigaltstack
The system-defined SIGSTKSZ is inadequate for Linux on modern x86_64
CPUs when handling multiple nested signal handlers and dynamic linking
The value is very much for a minimal viable stack, and is not enough
to run user-supplied python code. _Pythread_preferred_stack_size will
nominally return the stack size the system would use to create thrads.

@peadar peadar force-pushed the peadar:issue21131 branch from df80684 to 6d0757f Aug 12, 2019

@justbennet

This comment has been minimized.

Copy link

commented Aug 13, 2019

I just updated the Bug report at https://bugs.python.org/issue21131 to report that this only seems to apply when Python is configured to use shared libraries. Using

$ ./configure --prefix=/path/to/install

leads to make test passing all tests. But, using

$ ./configure --prefix=/path/to/install --enable-shared

leads to the faulthandler test failing. The patch file at the Python bugs issue will fix the problem, and the faulthandler test then succeeds.

I am not sure why that is, but it may explain some people's being unable to reproduce the problem.

@vstinner
Copy link
Member

left a comment

I'm not comfortable with this change since it modify the core thread module only to fix a faulthandler unit test. faulthandler uses SIGSTKSZ. It's not obvious to me how it relates to the "preferred thread stack size".

* Windows default stack size is 1MB, overridable by the linker in the
* executable
*/
return 1024 * 1024;

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 14, 2019

Member

I'm not comfortable with hardcoded values, especially the comment "overridable by the linker in the executable". Maybe we should get the size written by the linker in this case? We don't really need this function for faulthandler, maybe remove the Windows implementation?

This comment has been minimized.

Copy link
@peadar

peadar Aug 14, 2019

Author

Sure - I added it for completeness/documentation, but it's not called. I can remove it.

/* Note: This matches the value of -Wl,-stack_size in configure.ac */
#define THREAD_STACK_SIZE 0x1000000

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 14, 2019

Member

Previously, THREAD_STACK_SIZE was redefined if THREAD_STACK_SIZE=0. With your patch, it seems like it's always overriden: THREAD_STACK_SIZE value is no longer tested. I would prefer to minimize changes in this PR.

This comment has been minimized.

Copy link
@peadar

peadar Aug 14, 2019

Author

The end result is the same, but with less code - Originally THREAD_STACK_SIZE was always defined to 0 if not previously defined, and then was undefined/redefined if it was 0 in response to the platform detection macros. The code here was really just trying to extract the platform's preferred stack size (as reported by pthreads), and to not replicate this between the threads and faulthandler modules. Let me minimize the diff then.

Show resolved Hide resolved Include/internal/pycore_thread.h Outdated
@@ -496,3 +496,13 @@ PyThread_tss_get(Py_tss_t *key)
SetLastError(error);
return result;
}

size_t
_PyThread_preferred_stacksize()

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 14, 2019

Member
Suggested change
_PyThread_preferred_stacksize()
_PyThread_preferred_stacksize(void)
Show resolved Hide resolved Python/thread_pthread.h Outdated
@bedevere-bot

This comment has been minimized.

Copy link

commented Aug 14, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

peadar and others added some commits Aug 14, 2019

Update Include/internal/pycore_thread.h
Co-Authored-By: Victor Stinner <vstinner@redhat.com>
Update Python/thread_pthread.h
Co-Authored-By: Victor Stinner <vstinner@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.