Skip to content

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

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions lib/posix/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,36 @@ config SEM_VALUE_MAX
help
Maximum semaphore count in POSIX compliant Application.

config PTHREAD_DYNAMIC_STACK
bool "Support for dynamic stacks"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

default y
help
POSIX 1003.1 allows a NULL pthread_attr_t* to be passed to
pthread_create(3). However, Zephyr has traditionally required
that the caller statically allocate a stack and pass it in via the
pthread_attr_t*. With this option selected, NULL will be permitted
and a suitable stack will be automatically allocated and assigned,
inheriting permissions from the calling thread.

if PTHREAD_DYNAMIC_STACK
config PTHREAD_DYNAMIC_STACK_SIZE
int "Dynamic pthread stack size (in bytes)"
default 2048
range 1 4096
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And yeah, why 4096 is upper bound?

Copy link
Member Author

@cfriedt cfriedt Jul 24, 2020

Choose a reason for hiding this comment

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

@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?

help
Fix the size of dynamically-allocated stacks to be this many bytes.

# May be zero in order to later facilitate exclusively heap-allocated stacks
# API is currently in development
config PTHREAD_DYNAMIC_STACK_RESERVED_COUNT
int "The number of statically allocated dynamically assignable stacks"
default 0
range 0 32
help
Statically allocate this many stacks at compile-time.

endif # PTHREAD_DYNAMIC_STACK

endif # PTHREAD_IPC

config POSIX_CLOCK
Expand Down
149 changes: 142 additions & 7 deletions lib/posix/pthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@ static const pthread_attr_t init_pthread_attrs = {
static struct posix_thread posix_thread_pool[CONFIG_MAX_PTHREAD_COUNT];
PTHREAD_MUTEX_DEFINE(pthread_pool_lock);

#if defined(CONFIG_PTHREAD_DYNAMIC_STACK) \
&& CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT > 0

/* 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;

#endif

static bool is_posix_prio_valid(uint32_t priority, int policy)
{
if (priority >= sched_get_priority_min(policy) &&
Expand Down Expand Up @@ -120,6 +132,121 @@ static void zephyr_thread_wrapper(void *arg1, void *arg2, void *arg3)
pthread_exit(NULL);
}

#if defined(CONFIG_PTHREAD_DYNAMIC_STACK)

static pthread_attr_t *zephry_pthread_attr_new_static(void)
{
pthread_attr_t *attr = NULL;

#if CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT > 0
int ret;
/* position of available attribute */
uint8_t pos;
/* bitmap of in use attributes (copy of dstack_attrs) */
uint32_t in_use;

pthread_mutex_lock(&pthread_pool_lock);

/* count how many bits are set in dstack_in_use */
in_use = dstack_in_use;
if (__builtin_popcount(in_use) == CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT) {
goto out;
}

for (pos = 0; 1 == (in_use & 1); pos++, in_use >>= 1)
;

/* initialize the stack */
ret = pthread_attr_init(&dstack_attrs[pos]);
__ASSERT_NO_MSG(ret == 0);

ret = pthread_attr_setstack(&dstack_attrs[pos], &dstack[pos],
CONFIG_PTHREAD_DYNAMIC_STACK_SIZE);
__ASSERT_NO_MSG(ret == 0);

/* finally, mark the stack as in use and update the output */
dstack_in_use |= (1 << pos);

attr = &dstack_attrs[pos];

out:
pthread_mutex_unlock(&pthread_pool_lock);

#endif
return attr;
}

static inline pthread_attr_t *zephry_pthread_attr_new_dynamic(void)
{
return NULL;
}

static pthread_attr_t *zephyr_pthread_attr_new(void)
{
pthread_attr_t *attr;

attr = zephry_pthread_attr_new_static();
if (attr == NULL) {
attr = zephry_pthread_attr_new_dynamic();
}

return attr;
}

static bool zephry_pthread_attr_delete_static(pthread_attr_t *attr)
{
#if CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT > 0
bool ret = false;
size_t pos;

pthread_mutex_lock(&pthread_pool_lock);

for (pos = 0; pos < CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT; ++pos) {
if (&dstack_attrs[pos] == attr) {
dstack_in_use &= ~(1 << pos);
ret = true;
break;
}
}

pthread_mutex_unlock(&pthread_pool_lock);

return ret;
#else
ARG_UNUSED(attr);
return false;
#endif
}

static inline bool zephyr_pthread_attr_delete_dynamic(pthread_attr_t *attr)
{
ARG_UNUSED(attr);
return false;
}

static bool zephyr_pthread_attr_delete(pthread_attr_t *attr)
{
return
false
|| zephry_pthread_attr_delete_static(attr)
|| zephyr_pthread_attr_delete_dynamic(attr)
;
}

#else

static inline pthread_attr_t *zephyr_pthread_attr_new(void)
{
return NULL;
}

static inline bool zephyr_pthread_attr_delete(pthread_attr_t *attr)
{
ARG_UNUSED(attr);
return false;
}
#endif

/**
* @brief Create a new thread.
*
Expand All @@ -136,17 +263,24 @@ int pthread_create(pthread_t *newthread, const pthread_attr_t *attr,
pthread_condattr_t cond_attr;
struct posix_thread *thread;

/*
* FIXME: Pthread attribute must be non-null and it provides stack
* pointer and stack size. So even though POSIX 1003.1 spec accepts
* attrib as NULL but zephyr needs it initialized with valid stack.
*/
if ((attr == NULL) || (attr->initialized == 0U)
|| (attr->stack == NULL) || (attr->stacksize == 0)) {
if (attr != NULL && (attr->initialized == 0 || attr->stack == NULL
|| attr->stacksize == 0)) {
return EINVAL;
}

if (attr == NULL) {
if (IS_ENABLED(CONFIG_PTHREAD_DYNAMIC_STACK)) {
attr = zephyr_pthread_attr_new();
if (attr == NULL) {
return EAGAIN;
}
} else {
return EINVAL;
}
}

pthread_mutex_lock(&pthread_pool_lock);

for (pthread_num = 0;
pthread_num < CONFIG_MAX_PTHREAD_COUNT; pthread_num++) {
thread = &posix_thread_pool[pthread_num];
Expand All @@ -158,6 +292,7 @@ int pthread_create(pthread_t *newthread, const pthread_attr_t *attr,
pthread_mutex_unlock(&pthread_pool_lock);

if (pthread_num >= CONFIG_MAX_PTHREAD_COUNT) {
zephyr_pthread_attr_delete((pthread_attr_t *)attr);
return EAGAIN;
}

Expand Down
2 changes: 1 addition & 1 deletion tests/posix/common/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ CONFIG_ZTEST=y
CONFIG_SEM_VALUE_MAX=32767
CONFIG_POSIX_MQUEUE=y
CONFIG_HEAP_MEM_POOL_SIZE=4096
CONFIG_MAX_THREAD_BYTES=4
CONFIG_MAX_THREAD_BYTES=2
CONFIG_THREAD_NAME=y

CONFIG_SMP=n
39 changes: 27 additions & 12 deletions tests/posix/common/src/pthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ void *thread_top_term(void *p1)
}

if (id >= 2) {
if (IS_ENABLED(CONFIG_PTHREAD_DYNAMIC_STACK)) {
zassert_false(pthread_detach(self), "failed to set detach state");
}
ret = pthread_detach(self);
if (id == 2) {
zassert_equal(ret, EINVAL, "re-detached thread!");
Expand Down Expand Up @@ -301,18 +304,20 @@ void test_posix_pthread_execution(void)
ret = pthread_setname_np(NULL, thr_name);
zassert_equal(ret, ESRCH, "uninitialized setname!");

/* TESTPOINT: Try creating thread before attr init */
ret = pthread_create(&newthread[0], &attr[0],
thread_top_exec, NULL);
zassert_equal(ret, EINVAL, "thread created before attr init!");
if (!IS_ENABLED(CONFIG_PTHREAD_DYNAMIC_STACK)) {
/* TESTPOINT: Try creating thread before attr init */
ret = pthread_create(&newthread[0], &attr[0],
thread_top_exec, NULL);
zassert_equal(ret, EINVAL, "thread created before attr init!");
}

for (i = 0; i < N_THR_E; i++) {
ret = pthread_attr_init(&attr[i]);
if (ret != 0) {
zassert_false(pthread_attr_destroy(&attr[i]),
"Unable to destroy pthread object attrib");
"Unable to destroy pthread object attrib");
zassert_false(pthread_attr_init(&attr[i]),
"Unable to create pthread object attrib");
"Unable to create pthread object attrib");
}

/* TESTPOINTS: Retrieve set stack attributes and compare */
Expand All @@ -333,11 +338,16 @@ void test_posix_pthread_execution(void)
pthread_attr_setschedparam(&attr[i], &schedparam);
pthread_attr_getschedparam(&attr[i], &getschedparam);
zassert_equal(schedparam.sched_priority,
getschedparam.sched_priority,
"scheduling priorities do not match!");
getschedparam.sched_priority,
"scheduling priorities do not match!");

ret = pthread_create(&newthread[i], &attr[i], thread_top_exec,
INT_TO_POINTER(i));
if (IS_ENABLED(CONFIG_PTHREAD_DYNAMIC_STACK)) {
ret = pthread_create(&newthread[i], NULL, thread_top_exec,
INT_TO_POINTER(i));
} else {
ret = pthread_create(&newthread[i], &attr[i], thread_top_exec,
INT_TO_POINTER(i));
}

/* TESTPOINT: Check if thread is created successfully */
zassert_false(ret, "Number of threads exceed max limit");
Expand Down Expand Up @@ -429,8 +439,13 @@ void test_posix_pthread_termination(void)
schedparam.sched_priority = 2;
pthread_attr_setschedparam(&attr[i], &schedparam);
pthread_attr_setstack(&attr[i], &stack_t[i][0], STACKS);
ret = pthread_create(&newthread[i], &attr[i], thread_top_term,
INT_TO_POINTER(i));
if (IS_ENABLED(CONFIG_PTHREAD_DYNAMIC_STACK)) {
ret = pthread_create(&newthread[i], NULL, thread_top_term,
INT_TO_POINTER(i));
} else {
ret = pthread_create(&newthread[i], &attr[i], thread_top_term,
INT_TO_POINTER(i));
}

zassert_false(ret, "Not enough space to create new thread");
}
Expand Down
13 changes: 13 additions & 0 deletions tests/posix/common/testcase.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,24 @@ tests:
platform_exclude: nsim_sem_mpu_stack_guard nsim_em_mpu_stack_guard
extra_configs:
- CONFIG_NEWLIB_LIBC=n
portability.posix.common.pthread.dynamic_stack:
platform_exclude: nsim_sem_mpu_stack_guard nsim_em_mpu_stack_guard
extra_configs:
- CONFIG_NEWLIB_LIBC=n
- CONFIG_PTHREAD_DYNAMIC_STACK=y
- CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT=8
portability.posix.common.newlib:
platform_exclude: nsim_sem_mpu_stack_guard nsim_em_mpu_stack_guard
filter: TOOLCHAIN_HAS_NEWLIB == 1
extra_configs:
- CONFIG_NEWLIB_LIBC=y
portability.posix.common.newlib.pthread.dynamic_stack:
platform_exclude: nsim_sem_mpu_stack_guard nsim_em_mpu_stack_guard
filter: TOOLCHAIN_HAS_NEWLIB == 1
extra_configs:
- CONFIG_NEWLIB_LIBC=y
- CONFIG_PTHREAD_DYNAMIC_STACK=y
- CONFIG_PTHREAD_DYNAMIC_STACK_RESERVED_COUNT=8
portability.posix.common.nsim:
platform_whitelist: nsim_sem_mpu_stack_guard nsim_em_mpu_stack_guard
extra_configs:
Expand Down