From b3755b751ed42d98c933a919c744dc6455f5ee68 Mon Sep 17 00:00:00 2001 From: Sudeep Mohanty Date: Mon, 31 Oct 2022 10:02:03 +0100 Subject: [PATCH] pthread: Remove pthread TLS cleanup dependency on FreeRTOS Static Task Cleanup Hook This commit removes the need to define the vTaskCleanupTCB hook in pthread to cleanup the thread-specific data before a thread is deleted. --- components/freertos/Kconfig | 2 +- .../include/freertos/FreeRTOSConfig.h | 2 +- components/pthread/CMakeLists.txt | 4 -- components/pthread/pthread.c | 9 ++-- components/pthread/pthread_internal.h | 20 +++----- components/pthread/pthread_local_storage.c | 48 ++++++------------- .../main/test_pthread_local_storage.c | 46 +++++++++--------- .../pytest_pthread_unity_tests.py | 33 ++++++++++++- .../sdkconfig.ci.single_core_esp32_tls | 4 ++ .../pthread_unity_tests/sdkconfig.ci.tls | 2 + docs/en/api-reference/system/pthread.rst | 2 +- tools/ci/check_copyright_ignore.txt | 1 - 12 files changed, 90 insertions(+), 83 deletions(-) create mode 100644 components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.single_core_esp32_tls create mode 100644 components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.tls diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index bdb5a21b7c..3aede5edfc 100644 --- a/components/freertos/Kconfig +++ b/components/freertos/Kconfig @@ -293,7 +293,7 @@ menu "FreeRTOS" config FREERTOS_TLSP_DELETION_CALLBACKS bool "Enable thread local storage pointers deletion callbacks" - depends on FREERTOS_SMP && (FREERTOS_THREAD_LOCAL_STORAGE_POINTERS > 0) + depends on (FREERTOS_THREAD_LOCAL_STORAGE_POINTERS > 0) default y help ESP-IDF provides users with the ability to free TLSP memory by registering TLSP deletion callbacks. diff --git a/components/freertos/esp_additions/include/freertos/FreeRTOSConfig.h b/components/freertos/esp_additions/include/freertos/FreeRTOSConfig.h index 7d8e19f308..24238eddc9 100644 --- a/components/freertos/esp_additions/include/freertos/FreeRTOSConfig.h +++ b/components/freertos/esp_additions/include/freertos/FreeRTOSConfig.h @@ -266,7 +266,7 @@ Note: Include trace macros here and not above as trace macros are dependent on s // ---------------------- Features ------------------------- -#define configTHREAD_LOCAL_STORAGE_DELETE_CALLBACKS 1 +#define configTHREAD_LOCAL_STORAGE_DELETE_CALLBACKS CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS #ifndef configIDLE_TASK_STACK_SIZE #define configIDLE_TASK_STACK_SIZE CONFIG_FREERTOS_IDLE_TASK_STACKSIZE #endif diff --git a/components/pthread/CMakeLists.txt b/components/pthread/CMakeLists.txt index e7336108c0..fb66f7ca5a 100644 --- a/components/pthread/CMakeLists.txt +++ b/components/pthread/CMakeLists.txt @@ -13,10 +13,6 @@ list(APPEND extra_link_flags "-u pthread_include_pthread_cond_impl") list(APPEND extra_link_flags "-u pthread_include_pthread_local_storage_impl") list(APPEND extra_link_flags "-u pthread_include_pthread_rwlock_impl") -if(CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP AND NOT CONFIG_FREERTOS_SMP) # See IDF-4955 - target_link_libraries(${COMPONENT_LIB} INTERFACE "-Wl,--wrap=vPortCleanUpTCB") -endif() - if(extra_link_flags) target_link_libraries(${COMPONENT_LIB} INTERFACE "${extra_link_flags}") endif() diff --git a/components/pthread/pthread.c b/components/pthread/pthread.c index a48f59d30d..1d695bfd68 100644 --- a/components/pthread/pthread.c +++ b/components/pthread/pthread.c @@ -365,6 +365,8 @@ int pthread_join(pthread_t thread, void **retval) pthread_delete(pthread); xSemaphoreGive(s_threads_mux); } + /* clean up thread local storage before task deletion */ + pthread_internal_local_storage_destructor_callback(handle); vTaskDelete(handle); } @@ -399,6 +401,8 @@ int pthread_detach(pthread_t thread) } else { // pthread already stopped pthread_delete(pthread); + /* clean up thread local storage before task deletion */ + pthread_internal_local_storage_destructor_callback(handle); vTaskDelete(handle); } xSemaphoreGive(s_threads_mux); @@ -409,9 +413,8 @@ int pthread_detach(pthread_t thread) void pthread_exit(void *value_ptr) { bool detached = false; - /* preemptively clean up thread local storage, rather than - waiting for the idle task to clean up the thread */ - pthread_internal_local_storage_destructor_callback(); + /* clean up thread local storage before task deletion */ + pthread_internal_local_storage_destructor_callback(NULL); if (xSemaphoreTake(s_threads_mux, portMAX_DELAY) != pdTRUE) { assert(false && "Failed to lock threads list!"); diff --git a/components/pthread/pthread_internal.h b/components/pthread/pthread_internal.h index 80ddb89395..11c084d3c3 100644 --- a/components/pthread/pthread_internal.h +++ b/components/pthread/pthread_internal.h @@ -1,18 +1,10 @@ -// Copyright 2017 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at - -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2017-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #pragma once -void pthread_internal_local_storage_destructor_callback(void); +void pthread_internal_local_storage_destructor_callback(TaskHandle_t handle); extern portMUX_TYPE pthread_lazy_init_lock; diff --git a/components/pthread/pthread_local_storage.c b/components/pthread/pthread_local_storage.c index 98f5f88155..abb547675b 100644 --- a/components/pthread/pthread_local_storage.c +++ b/components/pthread/pthread_local_storage.c @@ -15,6 +15,11 @@ #include "pthread_internal.h" +/* Sanity check to ensure that the number of FreeRTOS TLSPs is at least 1 */ +#if (CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS < 1) + #error "CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS cannot be 0 for pthread TLS" +#endif + #define PTHREAD_TLS_INDEX 0 typedef void (*pthread_destructor_t)(void*); @@ -117,7 +122,7 @@ int pthread_key_delete(pthread_key_t key) - The destructor is always called in the context of the thread itself - which is important if the task then calls pthread_getspecific() or pthread_setspecific() to update the state further, as allowed for in the spec. */ -static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls) +static void pthread_cleanup_thread_specific_data_callback(int index, void *v_tls) { values_list_t *tls = (values_list_t *)v_tls; assert(tls != NULL); @@ -142,46 +147,23 @@ static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls free(tls); } -#if CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP // IDF-4955 -/* Called from FreeRTOS task delete hook */ -void pthread_local_storage_cleanup(TaskHandle_t task) -{ - void *tls = pvTaskGetThreadLocalStoragePointer(task, PTHREAD_TLS_INDEX); - if (tls != NULL) { - pthread_local_storage_thread_deleted_callback(PTHREAD_TLS_INDEX, tls); - vTaskSetThreadLocalStoragePointer(task, PTHREAD_TLS_INDEX, NULL); - } -} - -void __real_vPortCleanUpTCB(void *tcb); - -/* If static task cleanup hook is defined then its applications responsibility to define `vPortCleanUpTCB`. - Here we are wrapping it, so that we can do pthread specific TLS cleanup and then invoke application - real specific `vPortCleanUpTCB` */ -void __wrap_vPortCleanUpTCB(void *tcb) -{ - pthread_local_storage_cleanup(tcb); - __real_vPortCleanUpTCB(tcb); -} -#endif // CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP - /* this function called from pthread_task_func for "early" cleanup of TLS in a pthread */ -void pthread_internal_local_storage_destructor_callback(void) +void pthread_internal_local_storage_destructor_callback(TaskHandle_t handle) { - void *tls = pvTaskGetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX); + void *tls = pvTaskGetThreadLocalStoragePointer(handle, PTHREAD_TLS_INDEX); if (tls != NULL) { - pthread_local_storage_thread_deleted_callback(PTHREAD_TLS_INDEX, tls); + pthread_cleanup_thread_specific_data_callback(PTHREAD_TLS_INDEX, tls); /* remove the thread-local-storage pointer to avoid the idle task cleanup calling it again... */ -#if CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP // IDF-4955 +#if !defined(CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS) vTaskSetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX, NULL); #else vTaskSetThreadLocalStoragePointerAndDelCallback(NULL, PTHREAD_TLS_INDEX, NULL, NULL); -#endif // CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP +#endif /* CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS */ } } @@ -223,14 +205,14 @@ int pthread_setspecific(pthread_key_t key, const void *value) if (tls == NULL) { return ENOMEM; } -#if defined(CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP) +#if !defined(CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS) vTaskSetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX, tls); #else vTaskSetThreadLocalStoragePointerAndDelCallback(NULL, PTHREAD_TLS_INDEX, tls, - pthread_local_storage_thread_deleted_callback); -#endif + pthread_cleanup_thread_specific_data_callback); +#endif /* CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS */ } value_entry_t *entry = find_value(tls, key); @@ -255,7 +237,7 @@ int pthread_setspecific(pthread_key_t key, const void *value) // a destructor may call pthread_setspecific() to add a new non-NULL value // to the list, and this should be processed after all other entries. // - // See pthread_local_storage_thread_deleted_callback() + // See pthread_cleanup_thread_specific_data_callback() value_entry_t *last_entry = NULL; value_entry_t *it; SLIST_FOREACH(it, tls, next) { diff --git a/components/pthread/test_apps/pthread_unity_tests/main/test_pthread_local_storage.c b/components/pthread/test_apps/pthread_unity_tests/main/test_pthread_local_storage.c index 1ad96af5a5..eeb9e56550 100644 --- a/components/pthread/test_apps/pthread_unity_tests/main/test_pthread_local_storage.c +++ b/components/pthread/test_apps/pthread_unity_tests/main/test_pthread_local_storage.c @@ -12,7 +12,7 @@ #include "test_utils.h" #include "esp_random.h" -TEST_CASE("pthread local storage basics", "[pthread]") +TEST_CASE("pthread local storage basics", "[thread-specific]") { pthread_key_t key; TEST_ASSERT_EQUAL(0, pthread_key_create(&key, NULL)); @@ -35,7 +35,7 @@ TEST_CASE("pthread local storage basics", "[pthread]") TEST_ASSERT_EQUAL(0, pthread_key_delete(key)); } -TEST_CASE("pthread local storage unique keys", "[pthread]") +TEST_CASE("pthread local storage unique keys", "[thread-specific]") { const int NUM_KEYS = 10; pthread_key_t keys[NUM_KEYS]; @@ -63,7 +63,7 @@ static void *expected_destructor_ptr; static void *actual_destructor_ptr; static void *thread_test_pthread_destructor(void *); -TEST_CASE("pthread local storage destructor", "[pthread]") +TEST_CASE("pthread local storage destructor", "[thread-specific]") { pthread_t thread; pthread_key_t key = -1; @@ -84,9 +84,25 @@ TEST_CASE("pthread local storage destructor", "[pthread]") TEST_ASSERT_EQUAL(0, pthread_key_delete(key)); } +static void *thread_test_pthread_destructor(void *v_key) +{ + printf("Local storage thread running...\n"); + pthread_key_t key = (pthread_key_t) v_key; + expected_destructor_ptr = &key; // address of stack variable in the task... + pthread_setspecific(key, expected_destructor_ptr); + printf("Local storage thread done.\n"); + return NULL; +} + +static void test_pthread_destructor(void *value) +{ + actual_destructor_ptr = value; +} + +#if defined(CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS) static void task_test_pthread_destructor(void *v_key); -TEST_CASE("pthread local storage destructor in FreeRTOS task", "[pthread]") +TEST_CASE("pthread local storage destructor in FreeRTOS task", "[thread-specific]") { // Same as previous test case, but doesn't use pthread APIs therefore must wait // for the idle task to call the destructor @@ -112,29 +128,13 @@ TEST_CASE("pthread local storage destructor in FreeRTOS task", "[pthread]") TEST_ASSERT_EQUAL(0, pthread_key_delete(key)); } - - -static void *thread_test_pthread_destructor(void *v_key) -{ - printf("Local storage thread running...\n"); - pthread_key_t key = (pthread_key_t) v_key; - expected_destructor_ptr = &key; // address of stack variable in the task... - pthread_setspecific(key, expected_destructor_ptr); - printf("Local storage thread done.\n"); - return NULL; -} - -static void test_pthread_destructor(void *value) -{ - actual_destructor_ptr = value; -} - static void task_test_pthread_destructor(void *v_key) { /* call the pthread main routine, then delete ourselves... */ thread_test_pthread_destructor(v_key); vTaskDelete(NULL); } +#endif /* CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS */ #define STRESS_NUMITER 2000000 #define STRESS_NUMTASKS 16 @@ -155,7 +155,7 @@ static void *thread_stress_test(void *v_key) // This test case added to reproduce issues with unpinned tasks and TLS -TEST_CASE("pthread local storage stress test", "[pthread]") +TEST_CASE("pthread local storage stress test", "[thread-specific]") { pthread_key_t key = -1; pthread_t threads[STRESS_NUMTASKS] = { 0 }; @@ -187,7 +187,7 @@ static void *s_test_repeat_destructor_thread(void *vp_state); // pthread_setspecific() to set another value when it runs, and also // // As described in https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html -TEST_CASE("pthread local storage 'repeat' destructor test", "[pthread]") +TEST_CASE("pthread local storage 'repeat' destructor test", "[thread-specific]") { int r; destr_test_state_t state = { .last_idx = -1 }; diff --git a/components/pthread/test_apps/pthread_unity_tests/pytest_pthread_unity_tests.py b/components/pthread/test_apps/pthread_unity_tests/pytest_pthread_unity_tests.py index 85722ea21b..9ca0589dea 100644 --- a/components/pthread/test_apps/pthread_unity_tests/pytest_pthread_unity_tests.py +++ b/components/pthread/test_apps/pthread_unity_tests/pytest_pthread_unity_tests.py @@ -16,7 +16,7 @@ from pytest_embedded import Dut ) def test_pthread(dut: Dut) -> None: dut.expect_exact('Press ENTER to see the list of tests') - dut.write('*') + dut.write('![thread-specific]') dut.expect_unity_test_output(timeout=300) @@ -31,5 +31,34 @@ def test_pthread(dut: Dut) -> None: ) def test_pthread_single_core(dut: Dut) -> None: dut.expect_exact('Press ENTER to see the list of tests') - dut.write('*') + dut.write('![thread-specific]') + dut.expect_unity_test_output(timeout=300) + + +@pytest.mark.generic +@pytest.mark.supported_targets +@pytest.mark.parametrize( + 'config', + [ + 'tls', + ], + indirect=True, +) +def test_pthread_tls(dut: Dut) -> None: + dut.expect_exact('Press ENTER to see the list of tests') + dut.write('[thread-specific]') + dut.expect_unity_test_output(timeout=300) + + +@pytest.mark.generic +@pytest.mark.parametrize( + 'config', + [ + pytest.param('single_core_esp32_tls', marks=[pytest.mark.esp32]), + ], + indirect=True, +) +def test_pthread_single_core_tls(dut: Dut) -> None: + dut.expect_exact('Press ENTER to see the list of tests') + dut.write('[thread-specific]') dut.expect_unity_test_output(timeout=300) diff --git a/components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.single_core_esp32_tls b/components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.single_core_esp32_tls new file mode 100644 index 0000000000..e330845339 --- /dev/null +++ b/components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.single_core_esp32_tls @@ -0,0 +1,4 @@ +CONFIG_IDF_TARGET="esp32" +CONFIG_FREERTOS_UNICORE=y +CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS=1 +CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS=y diff --git a/components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.tls b/components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.tls new file mode 100644 index 0000000000..53d47d4e9f --- /dev/null +++ b/components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.tls @@ -0,0 +1,2 @@ +CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS=1 +CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS=y diff --git a/docs/en/api-reference/system/pthread.rst b/docs/en/api-reference/system/pthread.rst index 0aab3b1f8f..1e642d0e86 100644 --- a/docs/en/api-reference/system/pthread.rst +++ b/docs/en/api-reference/system/pthread.rst @@ -120,7 +120,7 @@ Thread-Specific Data * ``pthread_key_delete()`` * ``pthread_setspecific()`` / ``pthread_getspecific()`` -.. note:: These functions can be called from tasks created using either pthread or FreeRTOS APIs +.. note:: These functions can be called from tasks created using either pthread or FreeRTOS APIs. When calling these functions from tasks created using FreeRTOS APIs, :ref:`CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS` config option must be enabled to ensure the thread-specific data is cleaned up before the task is deleted. .. note:: There are other options for thread local storage in ESP-IDF, including options with higher performance. See :doc:`/api-guides/thread-local-storage`. diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index ec0891b177..bfd7381398 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -884,7 +884,6 @@ components/openthread/include/esp_openthread_lock.h components/protocomm/include/transports/protocomm_console.h components/protocomm/include/transports/protocomm_httpd.h components/pthread/pthread_cond_var.c -components/pthread/pthread_internal.h components/pthread/test/test_cxx_cond_var.cpp components/pthread/test/test_cxx_std_future.cpp components/pthread/test/test_pthread.c