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

External Memory for Pthread (IDFGH-9237) #10623

Closed
wants to merge 2 commits into from
Closed

External Memory for Pthread (IDFGH-9237) #10623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e73e7a
This will become the Feature https://github.com/espressif/esp-idf/iss…
f-hoepfinger-hr-agrartechnik Feb 4, 2023
9a4a14d
seems vTaskDelete must be before pthread_delete otherwise a assert fa…
f-hoepfinger-hr-agrartechnik Feb 9, 2023
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
1 change: 1 addition & 0 deletions components/pthread/include/esp_pthread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ extern "C" {
/** pthread configuration structure that influences pthread creation */
typedef struct {
size_t stack_size; ///< The stack size of the pthread
uint32_t stack_alloc_caps; ///< A bit mask of memory capabilities (MALLOC_CAPS*) to use when allocating the stack. If zero, MALLOC_CAP_DEFAULT is assumed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this might cause trouble for users who initialized each member of esp_pthread_cfg_t manually like this:

    esp_pthread_cfg_t cfg;
    cfg.thread_name = name;
    cfg.pin_to_core = core_id;
    cfg.stack_size = stack;
    cfg.prio = prio;
    cfg.inherit_cfg = false;

If they would upgrade IDF including this change, esp_pthread_cfg_t.stack_alloc_caps is uninitialized. Once the configuration is used, this would produce a crash at runtime at best or a way more subtle memory issue in the worst case.

I don't have a definitive solution for this yet. The esp_pthread_set_cfg makes it hard to introduce non-breaking configuration changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed that our documentation is explicitly advising users to zero-initialize configuration structures. You can consider this comment resolved, then.

size_t prio; ///< The thread's priority
bool inherit_cfg; ///< Inherit this configuration further
const char* thread_name; ///< The thread name.
Expand Down
72 changes: 51 additions & 21 deletions components/pthread/pthread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ typedef struct esp_pthread_entry {
bool detached; ///< True if pthread is detached
void *retval; ///< Value supplied to calling thread during join
void *task_arg; ///< Task arguments
StackType_t *stack_for_task; ///< Task stack
StaticTask_t *taskTC; ///< Task taskTC
} esp_pthread_t;

/** pthread wrapper task arg */
Expand Down Expand Up @@ -121,6 +123,11 @@ static esp_pthread_t *pthread_find(TaskHandle_t task_handle)
static void pthread_delete(esp_pthread_t *pthread)
{
SLIST_REMOVE(&s_threads_list, pthread, esp_pthread_entry, list_node);
if (pthread->task_arg) {
free(pthread->task_arg);
}
free(pthread->stack_for_task);
free(pthread->taskTC);
free(pthread);
}

Expand Down Expand Up @@ -221,6 +228,7 @@ int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
}

uint32_t stack_size = CONFIG_PTHREAD_TASK_STACK_SIZE_DEFAULT;
uint32_t stack_alloc_caps = MALLOC_CAP_DEFAULT;
BaseType_t prio = CONFIG_PTHREAD_TASK_PRIO_DEFAULT;
BaseType_t core_id = get_default_pthread_core();
const char *task_name = CONFIG_PTHREAD_TASK_NAME_DEFAULT;
Expand All @@ -230,6 +238,10 @@ int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
if (pthread_cfg->stack_size) {
stack_size = pthread_cfg->stack_size;
}
if(pthread_cfg->stack_alloc_caps){
stack_alloc_caps = pthread_cfg->stack_alloc_caps;
}

if (pthread_cfg->prio && pthread_cfg->prio < configMAX_PRIORITIES) {
prio = pthread_cfg->prio;
}
Expand Down Expand Up @@ -272,27 +284,44 @@ int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
task_arg->func = start_routine;
task_arg->arg = arg;
pthread->task_arg = task_arg;
BaseType_t res = xTaskCreatePinnedToCore(&pthread_task_func,
// stack_size is in bytes. This transformation ensures that the units are
// transformed to the units used in FreeRTOS.
// Note: float division of ceil(m / n) ==
// integer division of (m + n - 1) / n
int size_stack = (stack_size + sizeof(StackType_t) - 1) / sizeof(StackType_t);
StackType_t *stack_for_task = (StackType_t *) heap_caps_calloc(1, size_stack, stack_alloc_caps | MALLOC_CAP_8BIT);
if (stack_for_task == NULL) {
ESP_LOGE(TAG, "Failed to allocate task stack!");
free(pthread);
free(task_arg);
return ENOMEM;
}
pthread->stack_for_task = stack_for_task;
StaticTask_t *taskTC = (StaticTask_t *) heap_caps_calloc(1, sizeof(StaticTask_t), MALLOC_CAP_INTERNAL|MALLOC_CAP_8BIT);
if (taskTC == NULL) {
ESP_LOGE(TAG, "Failed to create task!");
free(pthread);
free(task_arg);
free(stack_for_task);
return ENOMEM;
}
pthread->taskTC = taskTC;
xHandle = xTaskCreateStaticPinnedToCore(&pthread_task_func,
task_name,
// stack_size is in bytes. This transformation ensures that the units are
// transformed to the units used in FreeRTOS.
// Note: float division of ceil(m / n) ==
// integer division of (m + n - 1) / n
(stack_size + sizeof(StackType_t) - 1) / sizeof(StackType_t),
size_stack,
task_arg,
prio,
&xHandle,
stack_for_task,
taskTC,
core_id);

if (res != pdPASS) {
if (xHandle == NULL) {
ESP_LOGE(TAG, "Failed to create task!");
free(pthread);
free(task_arg);
if (res == errCOULD_NOT_ALLOCATE_REQUIRED_MEMORY) {
return ENOMEM;
} else {
return EAGAIN;
}
free(stack_for_task);
free(taskTC);
return EAGAIN;
}
pthread->handle = xHandle;

Expand Down Expand Up @@ -349,6 +378,9 @@ int pthread_join(pthread_t thread, void **retval)
wait = true;
} else { // thread has exited and task is already suspended, or about to be suspended
child_task_retval = pthread->retval;
/* clean up thread local storage before task deletion */
pthread_internal_local_storage_destructor_callback(handle);
vTaskDelete(handle);
pthread_delete(pthread);
}
}
Expand All @@ -362,12 +394,12 @@ int pthread_join(pthread_t thread, void **retval)
assert(false && "Failed to lock threads list!");
}
child_task_retval = pthread->retval;
/* clean up thread local storage before task deletion */
pthread_internal_local_storage_destructor_callback(handle);
vTaskDelete(handle);
pthread_delete(pthread);
xSemaphoreGive(s_threads_mux);
}
/* clean up thread local storage before task deletion */
pthread_internal_local_storage_destructor_callback(handle);
vTaskDelete(handle);
}

if (retval) {
Expand Down Expand Up @@ -400,10 +432,10 @@ int pthread_detach(pthread_t thread)
pthread->detached = true;
} else {
// pthread already stopped
vTaskDelete(handle);
pthread_delete(pthread);
/* clean up thread local storage before task deletion */
pthread_internal_local_storage_destructor_callback(handle);
vTaskDelete(handle);
}
xSemaphoreGive(s_threads_mux);
ESP_LOGV(TAG, "%s %p EXIT %d", __FUNCTION__, pthread, ret);
Expand All @@ -423,11 +455,9 @@ void pthread_exit(void *value_ptr)
if (!pthread) {
assert(false && "Failed to find pthread for current task!");
}
if (pthread->task_arg) {
free(pthread->task_arg);
}
if (pthread->detached) {
// auto-free for detached threads
vTaskDelete(NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for moving vTaskDelete(NULL) here? It seems that the semantics are changed significantly: the following two statements won't be executed, the log call and giving the semaphore below won't be executed, either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please take a look at pthread unit tests. They produce a deadlock, likely due to moving the vTaskDelete() call here which prevents the mutex from being unlocked below (xSemaphoreGive(s_threads_mux);).

pthread_delete(pthread);
detached = true;
} else {
Expand All @@ -450,7 +480,7 @@ void pthread_exit(void *value_ptr)
// do anything that might lock (such as printing to stdout)

if (detached) {
vTaskDelete(NULL);

} else {
vTaskSuspend(NULL);
}
Expand Down