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.

Sign up for GitHub

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

Jump to bottom

posix: pthread: implement pthread_cleanup_push() / pop() #65700

Merged
merged 3 commits into from
Nov 29, 2023
Merged

posix: pthread: implement pthread_cleanup_push() / pop() #65700

merged 3 commits into from
Nov 29, 2023

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Nov 23, 2023 •
edited
Loading

pthread_cleanup_push() and pthread_cleanup_pop() are required by the POSIX_THREADS_BASE Option Group as detailed in Section E.1 of IEEE-1003.1-2017.

The POSIX_THREADS_BASE Option Group is required for PSE51, PSE52, PSE53, and PSE54 conformance, and is otherwise mandatory for any POSIX conforming system as per Section A.2.1.3 of IEEE-1003-1.2017.

Fixes #59940
Fixes #59941

See also https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cleanup_pop.html

@cfriedt cfriedt added area: Documentation area: Tests Issues related to a particular existing or missing test area: POSIX POSIX API Library labels Nov 23, 2023
@cfriedt cfriedt requested review from kartben and ycsin November 23, 2023 17:17
@cfriedt cfriedt force-pushed the pthread-cleanup-push branch from ade7e56 to 574bdfc Compare November 23, 2023 17:20
@cfriedt cfriedt force-pushed the pthread-cleanup-push branch 4 times, most recently from ac15e65 to 509612c Compare November 24, 2023 00:59
@cfriedt cfriedt marked this pull request as ready for review November 25, 2023 22:17
@cfriedt cfriedt requested a review from jgl-meta November 25, 2023 22:17
@zephyrbot zephyrbot added area: Portability Standard compliant code, toolchain abstraction area: CMSIS API Layer CMSIS-RTOS API Layer labels Nov 25, 2023
@cfriedt cfriedt force-pushed the pthread-cleanup-push branch from 509612c to e53580a Compare November 25, 2023 22:19
@kartben kartben removed the area: CMSIS API Layer CMSIS-RTOS API Layer label Nov 25, 2023
@cfriedt
Copy link
Member Author

cfriedt commented Nov 27, 2023

Cannot reproduce build errors locally. Will rebase.

@cfriedt cfriedt force-pushed the pthread-cleanup-push branch from e53580a to 30d90e1 Compare November 27, 2023 01:20
@zephyrbot zephyrbot added the area: CMSIS API Layer CMSIS-RTOS API Layer label Nov 27, 2023
@cfriedt cfriedt force-pushed the pthread-cleanup-push branch from 30d90e1 to 4a562b4 Compare November 27, 2023 01:21
@keith-packard
Copy link
Collaborator

keith-packard commented Nov 27, 2023 •
edited
Loading

Cannot reproduce build errors locally. Will rebase.

I can. I'm seeing an allocation failure in test_thrd_reuse when i == 27 when pthread_setspecific(__pthread_cleanup_key, &cleanup_head); is called. It doesn't look like that allocation is ever freed.

@keith-packard
Copy link
Collaborator

set CONFIG_HEAP_MEM_POOL_SIZE=8192 and the test passes, which sure looks like a pretty simple memory leak. I'll let you enjoy the debugging process here.

@cfriedt cfriedt force-pushed the pthread-cleanup-push branch from 4a562b4 to 8679d9f Compare November 27, 2023 23:05
@cfriedt
Copy link
Member Author

cfriedt commented Nov 27, 2023

Cannot reproduce build errors locally. Will rebase.

I can. I'm seeing an allocation failure in test_thrd_reuse when i == 27 when pthread_setspecific(__pthread_cleanup_key, &cleanup_head); is called. It doesn't look like that allocation is ever freed.

It's not ever freed. It doesn't actually count as one of the CONFIG_MAX_PTHREAD_KEY_COUNT keys that are user-allocatable.

I'm using a relatively old SDK version 0.16.0. Still not seeing failures (on qemu_x86 and qemu_riscv64).

@keith-packard
Copy link
Collaborator

Cannot reproduce build errors locally. Will rebase.

I can. I'm seeing an allocation failure in test_thrd_reuse when i == 27 when pthread_setspecific(__pthread_cleanup_key, &cleanup_head); is called. It doesn't look like that allocation is ever freed.

It's not ever freed. It doesn't actually count as one of the CONFIG_MAX_PTHREAD_KEY_COUNT keys that are user-allocatable.

No, the problem is that there is per-thread data allocated in this call and that never gets freed, so you end up running out of the kernel pool after you allocate 1024 bytes (by default) of these things.

I'm using a relatively old SDK version 0.16.0. Still not seeing failures (on qemu_x86 and qemu_riscv64).

I'm using 0.16.4 and getting failures. Might be good to give that a whirl?

@cfriedt
Copy link
Member Author

cfriedt commented Nov 28, 2023 •
edited
Loading

No, the problem is that there is per-thread data allocated in this call and that never gets freed, so you end up running out of the kernel pool after you allocate 1024 bytes (by default) of these things.

Ah - I see it now. Yeah, that's kind of an annoyance of using k_malloc() for this sort of thing. Some implementations of pthread_setspecific() actually just use a soft TLS area.

I wonder if there is a way to implement pthread_cleanup_push() / pop() without using pthread_setspecific() - e.g. just on the stack 🤔

Let me give this one another whirl.

@cfriedt cfriedt force-pushed the pthread-cleanup-push branch from 8679d9f to 45ec140 Compare November 28, 2023 18:34
@cfriedt cfriedt force-pushed the pthread-cleanup-push branch 3 times, most recently from a42c93f to 5571919 Compare November 28, 2023 18:43
@cfriedt
posix: pthread: implement pthread_cleanup_push() / pop()
927adc1 
pthread_cleanup_push() and pthread_cleanup_pop() are required
by the POSIX_THREADS_BASE Option Group as detailed in Section
E.1 of IEEE-1003.1-2017.

The POSIX_THREADS_BASE Option Group is required for PSE51,
PSE52, PSE53, and PSE54 conformance, and is otherwise mandatory
for any POSIX conforming system as per Section A.2.1.3 of
IEEE-1003-1.2017.

In this change, we require the addition of a dedicated
pthread_key_t that will not be available for applilcation usage.

Rather than including that as part of
CONFIG_MAX_PTHREAD_KEY_COUNT, we increase the storage by 1 in
order to be least invasive from the application perspective.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
@cfriedt
tests: posix: test pthread_cleanup_push() / pop()
01497f2 
Add a test for pthread_cleanup_push() and
pthread_cleanup_pop().

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
@cfriedt
doc: posix: mark pthread_cleanup_push() and pop() as supported
0813e0f 
Update docs to reflect additional support of
pthread_cleanup_push() and pthread_cleanup_pop().

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
@cfriedt cfriedt force-pushed the pthread-cleanup-push branch from 5571919 to 0813e0f Compare November 28, 2023 18:51
@cfriedt
Copy link
Member Author

cfriedt commented Nov 28, 2023

@keith-packard - should be ready now.

keith-packard
keith-packard reviewed Nov 28, 2023
View reviewed changes
Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking nice and clean now.

@carlescufi carlescufi merged commit e260201 into zephyrproject-rtos:main Nov 29, 2023
25 checks passed
@cfriedt cfriedt deleted the pthread-cleanup-push branch November 29, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhedberg jhedberg jhedberg approved these changes

@keith-packard keith-packard keith-packard approved these changes

@kartben kartben Awaiting requested review from kartben

@ycsin ycsin Awaiting requested review from ycsin

@nashif nashif Awaiting requested review from nashif

@jgl-meta jgl-meta Awaiting requested review from jgl-meta

Assignees

@cfriedt cfriedt

Labels
area: CMSIS API Layer CMSIS-RTOS API Layer area: Documentation area: Portability Standard compliant code, toolchain abstraction area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

posix: implement pthread_cleanup_push() posix: implement pthread_cleanup_pop()
6 participants
@cfriedt @keith-packard @jhedberg @kartben @carlescufi @zephyrbot