[WIP] lib: posix: auto allocate pthread stack if needed

[cfriedt]

This change aims to facilitate heap-allocated Zephyr thread stacks, and by extension, heap-allocated pthread stacks. Specifically, it allows the following API call to be made with attr == NULL.

int pthread_create(pthread_t *restrict thread,
       const pthread_attr_t *restrict attr,
       void *(*start_routine)(void*), void *restrict arg);

It looks relatively simple to implement. However, the main complication is that threads often abort themselves, which results in all sorts of errors - mmu, mpu, usage faults, etc.

It's possible that the best way to fix that is to append the heap-allocated thread stacks of all threads to a list, and then k_free() them from the idle thread - preferably after the idle thread terminates any terminated threads.

Fixes #25973

[pfalcon]

Sorry for delay with looking into this. Trying to do this now.

A few beginning comments:

1. The "description" here starts from somewhere "in the middle". Maybe the expectation that reviewers would pick up "intro" from individual commit messages and/or patch contents (like, Kconfig option descriptions), but I wish there was intro in the description, give a few sentences re: how "it" is now in Zephyr (making clear what is that "it" in the first place), how it's in "normal" POSIX systems, and what this PR tries to achieve.

2. Even from initial reading, there's some confusion with terminology of: "dynamic", "static", "heap". Usually, "dynamic" is the opposite of "static". And usually "dynamic memory allocation" is "heap memory allocation". So, usage of "dynamic" borders on being a misnomer. What you seem to call "dynamic" appear to be system-allocated stacks (vs user-allocated). So, I'd prefer them to be called such. In config options that may be long, but at least very true. (Another option might be "preallocated", but that's ambiguous too - preallocated by whom?)

[pfalcon]

I guess first step here is to clean up terminology.

[pfalcon]

Please spell it fully: "Support for dynamic thread stacks"

[cfriedt]

To be honest, @pfalcon, I don't even think there should be a Kconfig for this. It's basically synonymous with "are we allowed to pass a NULL attr?" The spec explicitly says "yes" here. Maybe removing this would make it easier to solve the static / dynamic wording as well?

I guess the way you're leaning is that dynamic <=> heap-allocated, while static <=> statically allocated?

It would be nice to get @andrewboie 's input as well for naming.

[pfalcon]

I don't think that "1" is realistic lower bound here. The standard (https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_attr_getstack.html) says:

The stacksize shall be at least {PTHREAD_STACK_MIN}.

[pfalcon]

And yeah, why 4096 is upper bound?

[cfriedt]

@pfalcon - the only reference I have in Zephyr for PTHREAD_STACK_MIN is in tests/posix/eventfd/src/main.c and it's only setting the min to 0 if it is undefined. Is there something analogous for Zephyr threads?

There is XT_STACK_MIN_SIZE for xtensa only.

Correct me if I'm wrong, but it would seem that Kconfig does not allow us to set a min attribute for int options. Otherwise, I would happily use min NNNN instead.

@andrewboie - any ideas?

[pfalcon]

I wish there was intro in the description, give a few sentences re: how "it" is now in Zephyr (making clear what is that "it" in the first place), how it's in "normal" POSIX systems, and what this PR tries to achieve.

Then of course it should proceed with specifics. For example, the descriptions here put emphasis on the case of attr ==NULL. But refreshing my memory of it (to remind, I'm not innately familiar with pthreads), I remember of pthread_attr_setstacksize() vs pthread_attr_setstack(). So, the implementation should differentiate cases of: a) pthread_attr_setstack() called, vs b) pthread_attr_setstacksize() was called, or not called (in which case, just default stack size is used).

All that leads to an obvious implementation, where one big chunk is pre-allocated, and stacks of different sizes are just sub-allocated from it. The description should explain why that path was not taken.

2nd thought is that all this static pre-allocation is useless, and instead, stacks should be just allocated from heap. Again, there should be description why that path is not taken, with links to prior art, etc.

[cfriedt]

All that leads to an obvious implementation, where one big chunk is pre-allocated, and stacks of different sizes are just sub-allocated from it. The description should explain why that path was not taken.

@pfalcon - that is, in fact, exactly what I added.

/* statically allocated, dynamically assigned stacks */
static K_THREAD_STACK_ARRAY_DEFINE(dstack,
	CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT,
	CONFIG_PTHREAD_DYNAMIC_STACK_SIZE);
static pthread_attr_t dstack_attrs[CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT];
static uint32_t dstack_in_use;

2nd thought is that all this static pre-allocation is useless, and instead, stacks should be just allocated from heap. Again, there should be description why that path is not taken, with links to prior art, etc.

When I originally wrote this change, there was no official way to dynamically allocate a thread stack. That also depended on C11 aligned_alloc(), which was added in ed258e9 by @andyross (btw, thanks Andy - C11 has some nice features!). For heap-allocated thread stacks, @andrewboie has a bit of work to do, but heap-allocated thread stacks should be possible soon.

With all that being said, there are only 2 reasons why I would consider statically-allocated (but dynamically assignable) thread stacks to be useful even with the availability of heap-allocated stacks:

1. When heap memory is exhausted or too fragmented to provide a contiguous region of memory, and a fairly important thread still needs to be spawned. With a pool of statically allocated thread stacks, this is still possible.
2. If some safety-critical standard precludes heap-allocation from being used (but API precludes a static thread stack from being provided), as is the case with C++ std::thread. While it would be ideal for native Zephyr threads to be used for C++11 threads, they could also be added today to Zephyr using the existing, conventional gthr-posix implementation with attr == NULL (ideally, newlib would support a generic C11 implementation too...).

However, I am quite open to modifying the PR to go entirely heap-allocated when attr == NULL, if that is the preferred way to do things.

[pfalcon]

that is, in fact, exactly what I added.

Then I miss that, because what I see is that you added preallocation of COUNT stack of fixed SIZE. Not, again, ability to allocate variable-sized stacks from common pool. If you did that on top of COUNT x SIZE thingy, then it's quite an advanced exploit of great Zephyr capabilities, which is again should be explained in the commit message. (I admit that that I didn't review the entire patchset closely, stopping when I figured that I get consistently misguided by the word DYNAMIC, and suggesting to work out terminology first).

[cfriedt]

that is, in fact, exactly what I added.

Then I miss that, because what I see is that you added preallocation of COUNT stack of fixed SIZE. Not, again, ability to allocate variable-sized stacks from common pool. If you did that on top of COUNT x SIZE thingy, then it's quite an advanced exploit of great Zephyr capabilities, which is again should be explained in the commit message. (I admit that that I didn't review the entire patchset closely, stopping when I figured that I get consistently misguided by the word DYNAMIC, and suggesting to work out terminology first).

I'm not concerned with variably-sized stacks at this time (it's a feature that could be supported later), but I think that would require heap-allocated stacks to work.

[pfalcon]

With all that being said, there are only 2 reasons why I would consider statically-allocated (but dynamically assignable) thread stacks to be useful even with the availability of heap-allocated stacks

Sounds good. But between the lines, the whole thing reads "these extravagant workarounds are added because Zephyr can't do otherwise", and I'd prefer to spell it out (if that holds), instead of "everybody knows that, but doesn't talk about it".

[pfalcon]

I'm not concerned with variably-sized stacks at this time (it's a feature that could be supported later), but I think that would require heap-allocated stacks to work.

Again, my concern is that we're adding workarounds here, instead of working towards more general solution (just mind that "important thread which may need to run anytime" usecase is already served with explicitly specified stack addr/size). I wanted to spell out that concern explicitly. If you think that's reasonable approach to do it so piecewise, well, ok. I don't have problems with incremental solutions.

[cfriedt]

I'm not concerned with variably-sized stacks at this time (it's a feature that could be supported later), but I think that would require heap-allocated stacks to work.

Again, my concern is that we're adding workarounds here, instead of working towards more general solution (just mind that "important thread which may need to run anytime" usecase is already served with explicitly specified stack addr/size). I wanted to spell out that concern explicitly. If you think that's reasonable approach to do it so piecewise, well, ok. I don't have problems with incremental solutions.

Actually, if using C++, then the "important thread which may need to run anytime" usecase is not served, as there is no language specification to be able to supply a stack - regardless of whether Zephyr threads or pthreads are being used.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.