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

Fix #1429, adjust pthread stack to account for TCB+TLS #1430

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Dec 1, 2023

Checklist (Please check before submitting)

Describe the contribution
The glibc pthread implementation puts additional data structures at the base of the stack. The size of these is not known, but the only thing to go by is PTHREAD_MIN_STACK -- which should always be enough to hold the required objects.

Instead of simply assuring that the stack is at least PTHREAD_MIN_STACK, add this to the requested stack instead. This in turn will ensure that the user always has at least their requested stack size available for actual use.

If the pthread implementation does not advertise a PTHREAD_MIN_STACK value, then just reserve 1 extra page.

Fixes #1429

Testing performed
Execute all tests, execute CFE

Expected behavior changes
Internal stack size is now larger than requested in the API call, which compensates for the overhead used by pthreads itself.

System(s) tested on
Debian

Additional context
In testing this, the stack size for CFS apps in the cfe_es_startup.scr file was set at 8192 bytes. On the test system, PTHREAD_MIN_STACK was 16384.

Before the change, all CFS tasks were getting total stack of 16384 bytes (PTHREAD_MIN_STACK), but with almost 10kB already used when the entry point was invoked, thus only leaving about 6kB usable by the app. In this test, the MD app would sometimes overrun its stack, depending on how it loaded its tables.

After this change, the same CFS tasks now get 24576 bytes of total stack (PTHREAD_MIN_STACK + requested stack[8192]). This leaves about 14kB of usable stack, which more than the requested amount, and thus apps shouldn't overrun if the stack requirement is calculated correctly.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

The glibc pthread implementation puts additional data structures at the
base of the stack.  The size of these is not known, but the only thing
to go by is PTHREAD_MIN_STACK -- which should always be enough to hold
the required objects.

Instead of simply assuring that the stack is at least PTHREAD_MIN_STACK,
add this to the requested stack instead.  This in turn will ensure that
the user always has at least their requested stack size available for
actual use.

If the pthread implementation does not advertise a PTHREAD_MIN_STACK
value, then just reserve 1 extra page.
*/
#ifndef PTHREAD_STACK_MIN
#define PTHREAD_STACK_MIN (8 * 1024)
#ifdef PTHREAD_STACK_MIN

Check notice

Code scanning / CodeQL-coding-standard

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Dec 1, 2023
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Dec 7, 2023
dzbaker added a commit to nasa/cFS that referenced this pull request Dec 12, 2023
*Combines:*

to_lab v2.5.0-rc4+dev75
ci_lab v2.5.0-rc4+dev81
sample_app v1.3.0-rc4+dev69
sch_lab v2.5.0-rc4+dev75
cFE v7.0.0-rc4+dev434
osal v6.0.0-rc4+dev247

**Includes:**

*to_lab*
- nasa/to_lab#176

*ci_lab*
- nasa/ci_lab#165

*sample_app*
- nasa/sample_app#220

*sch_lab*
- nasa/sch_lab#156

*cFE*
- nasa/cFE#2472
- nasa/cFE#2411
- nasa/cFE#2474

*osal*
- nasa/osal#1430

Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Isaac Rowe <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
@dzbaker dzbaker mentioned this pull request Dec 12, 2023
2 tasks
dzbaker added a commit to nasa/cFS that referenced this pull request Dec 12, 2023
*Combines:*

to_lab v2.5.0-rc4+dev75
ci_lab v2.5.0-rc4+dev81
sample_app v1.3.0-rc4+dev69
sch_lab v2.5.0-rc4+dev75
cFE v7.0.0-rc4+dev434
osal v6.0.0-rc4+dev247

**Includes:**

*to_lab*
- nasa/to_lab#176

*ci_lab*
- nasa/ci_lab#165

*sample_app*
- nasa/sample_app#220

*sch_lab*
- nasa/sch_lab#156

*cFE*
- nasa/cFE#2472
- nasa/cFE#2411
- nasa/cFE#2474

*osal*
- nasa/osal#1430

Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Isaac Rowe <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
dzbaker added a commit to nasa/cFS that referenced this pull request Dec 12, 2023
*Combines:*

to_lab v2.5.0-rc4+dev75
ci_lab v2.5.0-rc4+dev81
sample_app v1.3.0-rc4+dev69
sch_lab v2.5.0-rc4+dev75
cFE v7.0.0-rc4+dev434
osal v6.0.0-rc4+dev247

**Includes:**

*to_lab*
- nasa/to_lab#176

*ci_lab*
- nasa/ci_lab#165

*sample_app*
- nasa/sample_app#220

*sch_lab*
- nasa/sch_lab#156

*cFE*
- nasa/cFE#2472
- nasa/cFE#2411
- nasa/cFE#2474

*osal*
- nasa/osal#1430

Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Isaac Rowe <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
@dzbaker dzbaker merged commit 45a6d98 into nasa:main Dec 12, 2023
20 checks passed
dzbaker added a commit to nasa/cFS that referenced this pull request Dec 12, 2023
*Combines:*

to_lab v2.5.0-rc4+dev75
ci_lab v2.5.0-rc4+dev81
sample_app v1.3.0-rc4+dev69
sch_lab v2.5.0-rc4+dev75
cFE v7.0.0-rc4+dev434
osal v6.0.0-rc4+dev247

**Includes:**

*to_lab*
- nasa/to_lab#176

*ci_lab*
- nasa/ci_lab#165

*sample_app*
- nasa/sample_app#220

*sch_lab*
- nasa/sch_lab#156

*cFE*
- nasa/cFE#2472
- nasa/cFE#2411
- nasa/cFE#2474

*osal*
- nasa/osal#1430

Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Isaac Rowe <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
@jphickey jphickey deleted the fix-1429-adjust-stack branch May 29, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack size on POSIX needs to account for internal TCB+TLS
2 participants