lib: posix: auto allocate pthread stack if needed

[tgorochowik]

POSIX 1003.1 spec allows creating pthreads with attrib as NULL. In that
case, the stack needs to be auto-allocated.

Signed-off-by: Tomasz Gorochowik tgorochowik@antmicro.com

[pfalcon]

I'd prefer if the title was "lib: posix: auto allocate pthread stack if needed", though I may imagine that makes it too long for prescriptive checks we have.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[pfalcon]

Thanks for the patch, it's definitely helpful. But I really think that non-initialized thread attrs behavior should be clarified.

[pfalcon]

These double-parens look really weird, please don't inherit them from previous code you change. But bigger problem is that this check doesn't look correct. It could be such only if you take frivolous interpretation of this phrase from http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html:

The behavior is undefined if the value specified by the attr argument to pthread_create() does not refer to an initialized thread attributes object.

So, if such behavior is undefined, you could treat uninitialized attrs as NULL attrs. But this way, we only complicate our own life with handling more attrs, etc.

Please add attr != NULL && attr->initialized == 0 check where previous EINVAL return was, this should simplify and clarify behavior for now and future.

[tgorochowik]

You are right, it is better not to do anything when the behavior is undefined. This also allowed me to restore the test I originally removed.

[pfalcon]

@tgorochowik, And outside of "review-marks" comments: I wanted to post when your previous patch gets merged, but missed it, but here you just patch the related test... So, umm, well, we really need to start think about regression tests for this stuff. I know firsthand that writing tests complicate bootstrapping a subsystem (and we're effectively at this stage yet with posix in zephyr), but as we add more features, it becomes more and more important. And just a fresh example, when functionality carefully crafted by one developer was just erased by another: #16453 .

To clarify, I don't see myself adding -1 review due to lack of tests anytime soon, but definitely going to treat that stuff less relaxed on my devel side, and "promote" it with contributors.

[tgorochowik]

@pfalcon Thank you for your review! I just pushed an updated version.

To highlight the changes from the previous revision:

• I restored the EINVAL being returned for cases where the behavior is undefined
• I added the _setstacksize function implementation, so it is now possible to use automatically allocated stack with a requested size (but the default will be used if it's not specified)
• I created a larger if which just checks if the passed attrs are null or not, tje else block will be a good place to add handling other custom attributes from users if it's necessary
• I modified the tests to use one thread with an automatically allocated stack (just one thread to minimize the malloc arena requirements) - Hopefully the CI will pass with this change as I tested the test changes only on selected platforms.

Please let me know what you think about these changes

[pfalcon]

Thanks for going thru trouble of updating it. Let me skip "isn't there a way to do it differently?" questions and approve. But CI of course needs to be fixed.

[pfalcon]

"externally defined stack is to be used" might need an update now.

[pfalcon]

This seems to hit for 119 tests or so... ;-).

[jukkar]

To clarify, I don't see myself adding -1 review due to lack of tests anytime soon, but definitely going to treat that stuff less relaxed on my devel side, and "promote" it with contributors.

I think the default should be that the tests are submitted together with new functionality. Writing tests afterwards is just pita. It should be an exception if no tests are created for new stuff.

[pfalcon]

It should be an exception if no tests are created for new stuff.

Right. We should just weigh a chance of motivating people to contribute more by providing quicker turnaround vs discouraging them by arbitrarily blocking their PRs (arbitrarily because rule of "tests are submitted together" is far from being closely followed). I'm personally all for the encouraging vs discouraging choice.

And on technical side, writing tests for Zephyr is oftentimes a pita itself, so we should look to improve that (good selection of convenience and helper functions, existing tests maintained and improved to serve a good copy-paste base, etc.)

[jukkar]

Right. We should just weigh a chance of motivating people to contribute more by providing quicker turnaround vs discouraging them by arbitrarily blocking their PRs (arbitrarily because rule of "tests are submitted together" is far from being closely followed). I'm personally all for the encouraging vs discouraging choice.

We are just moving the pain later if the writing the tests is postponed.

And on technical side, writing tests for Zephyr is oftentimes a pita itself, so we should look to improve that (good selection of convenience and helper functions, existing tests maintained and improved to serve a good copy-paste base, etc.)

IMHO it is not "oftentimes" but more or less always PITA to write the tests. Fortunately we have lot of skeleton tests that can be utilized when creating new tests.

[pfalcon]

Ping @tgorochowik, can you please check CI failures here?

[andrewboie]

This is not going to work. There is a reason why all stacks have to be declared with K_THREAD_STACK_DEFINE(mystack, 4096) instead of just char mystack[4096]. Stack buffers have size/alignment restrictions which need to be handled, and additional memory may be reserved within the object for other data like hardware stack overflow guard regions.

You can't use calloc() to allocate a stack buffer, there's no way to enforce proper alignment. You're going to need to set up some pool of available stacks with K_THREAD_STACK_ARRAY_DEFINE() and have some code to manage unused stack buffers within that pool.

In addition, your changes to the test code in tests/posix/common are not sufficient. This is not getting merged until you prove that this patch actually works in the test case.

[cfriedt]

Interesting. I had a similar proposal in #25973, but was a bit more conservative with it, suggesting that static allocation be explored before heap allocation.

E.g.

CONFIG_PTHREAD_DYNAMIC_STACK=y
CONFIG_PTHREAD_DYNAMIC_STACK_SIZE=512
CONFIG_PTHREAD_MAX_DYNAMIC_STACKS=2

and using K_THREAD_STACK_ARRAY_DEFINE(), which would likely make @andrewboie a bit happier (although I believe thread permissions will still need some attention).

@tgorochowik - would you be interested in combining our approaches?

Some alternatives I considered were EITHER static OR heap allocation, as well as a hybrid approach that first consumed statically declared stacks, and then went to heap allocation if those stacks were exhausted.

Personally, I feel that static allocation would be more friendly on small devices with only 10s of kBs of SRAM.

[tgorochowik]

@cfriedt unfortunately I don't have any resources to continue this at the moment, please feel free to take over or extract anything you find useful here and use it for your implementation, thanks for pinging me!

[cfriedt]

@tgorochowik Cool - I have a direct use for this, so I might do that. Thanks!