posix: abstract pthread_mutex_t as uint32_t #52087

cfriedt · 2022-11-08T19:52:56Z

3 commits

posix: pthread: take care with pthread mutex resources
posix: mutex: abstract pthread_mutex_t as uint32_t
tests: posix: mutex: test to ensure there is no resource leakage

See individual commit messages for details.

cfriedt · 2022-11-08T20:57:29Z

Compliance check failure is a false positive (pthread functions return non-negative errno values)
https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html

cfriedt · 2022-11-09T04:20:52Z

improved to_posix_mutex() (use the MSB of pthread_mutex_t to indicate it has been initialized)

Previously, `pthread_mutex_init()` could not actually fail, and destroying mutexes was a no-op, so it was missing in a couple of places. However, with the change of `pthread_mutex_t` to `uint32_t`, and since those are embedded inside of `struct posix_thread` for the time being, the pthread code needs to keep track that it is relinquishes used mutex resources when a thread completes. Signed-off-by: Chris Friedt <[email protected]>

cfriedt · 2022-11-11T11:40:23Z

lib/posix/pthread_mutex.c

+
+	/* Record the associated posix_mutex in mu and mark as initialized */
+	*mu = mark_pthread_mutex_initialized(bit);
+


This also needs to do the same initialization that pthread_mutex_init() does. Actually, it should probably just be moved here

Done! (mostly, custom attributes are still applied in pthread_mutex_init()

cfriedt · 2022-11-11T12:49:34Z

move mutex initialization to to_posix_mutex()
use to_posix_mutex() instead of get_posix_mutex() in cond_wait() because passing in mutexes initialized with PTHREAD_MUTEX_INITIALIZER is a valid use case.

Consistent with the change of `pthread_t` from `struct posix_thread` to `uint32_t`, we can now also abstract `pthread_mutex_t` as `uint32_t` and separate `struct posix_mutex` as an implementation detail, hidden from POSIX API consumers. This change deprecates `PTHREAD_MUTEX_DEFINE()` in favour of the (standardized) `PTHREAD_MUTEX_INITIALIZER`. This change introduces `CONFIG_MAX_PTHREAD_MUTEX_COUNT`. Signed-off-by: Chris Friedt <[email protected]>

Add a test to ensure that `pthread_mutex_t` can be used over and over again and that there is no resource leakage with proper usage. Signed-off-by: Chris Friedt <[email protected]>

cfriedt · 2022-11-13T23:01:06Z

add test test_posix_mutex_resource_exhausted()
add doxygen for test_posix_mutex_resource_exhausted(), test_posix_mutex_resource_leak()

cfriedt · 2022-11-14T18:29:05Z

@stephanosio - will need your superpowers to merge due to the compliance false positives.

belussi · 2022-11-14T20:11:38Z

Will this solve this problem? #35986

cfriedt · 2022-11-14T20:28:11Z

Will this solve this problem? #35986

@belussi - Yes (partially). I've been fixing it in increments for the last 2 weeks or so

belussi · 2022-11-15T08:32:54Z

@cfriedt Good to hear that! Looks like POSIX implementation is a kind of blocker for using POSIX and C++ STL.

cfriedt · 2022-11-15T14:23:01Z

@cfriedt Good to hear that! Looks like POSIX implementation is a kind of blocker for using POSIX and C++ STL.

@belussi - the C++ STL works already and has very little dependency on POSIX (if any).

However, Newlib's (prototypical) c++11 thread support uses pthreads and requires automatic stack allocation.

That in itself is not so hard, but it really should be solved at the kernel (Zephyr) layer so that other threading implementations can simply re-use the same machinery.

That will eventually will be fixed by #44379.

Later, we can move to a direct c++ threading implementation without POSIX.

PatrickM-ZS · 2022-11-15T17:46:16Z

If this relates to #35986 could that issue be re-opened? I'm not using any C++ in my project. For now I've just commented out declarations of the conflicting types in _pthreadtypes.h but i'm keen to find a proper workaround, until i guess the newlib headers are introduced as mentioned on the POSIX roadmap (#51211)

belussi · 2022-11-15T18:49:41Z

@PatrickM-ZS @cfriedt
We are seeing exactly the same issue as mentioned in #35986. Conflicting types error. I couldn't find any solution yet but will try removing _pthreadtypes.h, so only STL's POSIX declaration is seen.

stephanosio · 2022-11-16T09:43:41Z

Please open a discussion instead of commenting on old pull requests.

cfriedt requested review from keith-packard, enjiamai, stephanosio and KangJianX November 8, 2022 19:52

cfriedt force-pushed the use-uint32-for-pthread-mutex branch 5 times, most recently from 972d09b to c675a04 Compare November 8, 2022 20:06

cfriedt marked this pull request as ready for review November 8, 2022 20:57

cfriedt requested a review from pfalcon as a code owner November 8, 2022 20:57

zephyrbot added the area: POSIX POSIX API Library label Nov 8, 2022

zephyrbot assigned cfriedt Nov 8, 2022

cfriedt force-pushed the use-uint32-for-pthread-mutex branch from c675a04 to 8d872af Compare November 8, 2022 21:43

cfriedt force-pushed the use-uint32-for-pthread-mutex branch 4 times, most recently from 1c7c060 to 923a81d Compare November 9, 2022 04:44

cfriedt marked this pull request as draft November 9, 2022 14:35

cfriedt force-pushed the use-uint32-for-pthread-mutex branch 9 times, most recently from e27b2d8 to 1f1d9df Compare November 10, 2022 14:58

jeremybettis previously approved these changes Nov 10, 2022

View reviewed changes

cfriedt commented Nov 11, 2022

View reviewed changes

cfriedt mentioned this pull request Nov 11, 2022

posix: abstract pthread_cond_t as uint32_t #52173

Merged

cfriedt dismissed jeremybettis’s stale review via 5d32773 November 11, 2022 12:17

cfriedt force-pushed the use-uint32-for-pthread-mutex branch 2 times, most recently from 5d32773 to dd4195f Compare November 11, 2022 12:48

cfriedt force-pushed the use-uint32-for-pthread-mutex branch from dd4195f to eeccc88 Compare November 11, 2022 15:18

cfriedt requested a review from jeremybettis November 13, 2022 22:45

tests: posix: mutex: test to ensure there is no resource leakage

2477815

Add a test to ensure that `pthread_mutex_t` can be used over and over again and that there is no resource leakage with proper usage. Signed-off-by: Chris Friedt <[email protected]>

cfriedt force-pushed the use-uint32-for-pthread-mutex branch from eeccc88 to 2477815 Compare November 13, 2022 23:00

cfriedt mentioned this pull request Nov 13, 2022

POSIX Roadmap for LTSv3 #51211

Open

jeremybettis approved these changes Nov 14, 2022

View reviewed changes

mbolivar-nordic approved these changes Nov 14, 2022

View reviewed changes

PatrickM-ZS mentioned this pull request Nov 14, 2022

POSIX: multiple definition of posix_types #35986

Closed

stephanosio approved these changes Nov 14, 2022

View reviewed changes

stephanosio merged commit 47d09c0 into zephyrproject-rtos:main Nov 14, 2022

cfriedt deleted the use-uint32-for-pthread-mutex branch November 14, 2022 20:58

cfriedt mentioned this pull request Nov 17, 2022

Upstream Checklist zephyrproject-rtos/gsoc-2022-thrift#147

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

posix: abstract pthread_mutex_t as uint32_t #52087

posix: abstract pthread_mutex_t as uint32_t #52087

cfriedt commented Nov 8, 2022 •

edited

Loading

cfriedt commented Nov 8, 2022 •

edited

Loading

cfriedt commented Nov 9, 2022 •

edited

Loading

cfriedt Nov 11, 2022 •

edited

Loading

cfriedt Nov 11, 2022

cfriedt commented Nov 11, 2022 •

edited

Loading

cfriedt commented Nov 13, 2022

cfriedt commented Nov 14, 2022

belussi commented Nov 14, 2022 •

edited

Loading

cfriedt commented Nov 14, 2022 •

edited

Loading

belussi commented Nov 15, 2022

cfriedt commented Nov 15, 2022 •

edited

Loading

PatrickM-ZS commented Nov 15, 2022

belussi commented Nov 15, 2022

stephanosio commented Nov 16, 2022


		/* Record the associated posix_mutex in mu and mark as initialized */
		*mu = mark_pthread_mutex_initialized(bit);

posix: abstract pthread_mutex_t as uint32_t #52087

posix: abstract pthread_mutex_t as uint32_t #52087

Conversation

cfriedt commented Nov 8, 2022 • edited Loading

cfriedt commented Nov 8, 2022 • edited Loading

cfriedt commented Nov 9, 2022 • edited Loading

cfriedt Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

cfriedt Nov 11, 2022

Choose a reason for hiding this comment

cfriedt commented Nov 11, 2022 • edited Loading

cfriedt commented Nov 13, 2022

cfriedt commented Nov 14, 2022

belussi commented Nov 14, 2022 • edited Loading

cfriedt commented Nov 14, 2022 • edited Loading

belussi commented Nov 15, 2022

cfriedt commented Nov 15, 2022 • edited Loading

PatrickM-ZS commented Nov 15, 2022

belussi commented Nov 15, 2022

stephanosio commented Nov 16, 2022

cfriedt commented Nov 8, 2022 •

edited

Loading

cfriedt commented Nov 8, 2022 •

edited

Loading

cfriedt commented Nov 9, 2022 •

edited

Loading

cfriedt Nov 11, 2022 •

edited

Loading

cfriedt commented Nov 11, 2022 •

edited

Loading

belussi commented Nov 14, 2022 •

edited

Loading

cfriedt commented Nov 14, 2022 •

edited

Loading

cfriedt commented Nov 15, 2022 •

edited

Loading