From 26ffb63f74cd44d373a38f4fc49bb66ec613bbf7 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Sun, 23 Apr 2023 22:30:11 -0400 Subject: [PATCH 1/2] posix: sched: ensure min and max priority are schedulable Previously, there was an off-by-one error for SCHED_RR. Fixes #56729 Signed-off-by: Chris Friedt (cherry picked from commit 2b2cbf81076056c4fc35326c2ebb1d83cc75e7a7) --- lib/posix/pthread.c | 12 ++++++---- lib/posix/pthread_sched.c | 48 +++++++-------------------------------- 2 files changed, 15 insertions(+), 45 deletions(-) diff --git a/lib/posix/pthread.c b/lib/posix/pthread.c index 3829f36e750e..9c2c2ddbaa36 100644 --- a/lib/posix/pthread.c +++ b/lib/posix/pthread.c @@ -15,12 +15,10 @@ #define PTHREAD_INIT_FLAGS PTHREAD_CANCEL_ENABLE #define PTHREAD_CANCELED ((void *) -1) -#define LOWEST_POSIX_THREAD_PRIORITY 1 - PTHREAD_MUTEX_DEFINE(pthread_key_lock); static const pthread_attr_t init_pthread_attrs = { - .priority = LOWEST_POSIX_THREAD_PRIORITY, + .priority = 0, .stack = NULL, .stacksize = 0, .flags = PTHREAD_INIT_FLAGS, @@ -54,9 +52,11 @@ static uint32_t zephyr_to_posix_priority(int32_t z_prio, int *policy) if (z_prio < 0) { *policy = SCHED_FIFO; prio = -1 * (z_prio + 1); + __ASSERT_NO_MSG(prio < CONFIG_NUM_COOP_PRIORITIES); } else { *policy = SCHED_RR; - prio = (CONFIG_NUM_PREEMPT_PRIORITIES - z_prio); + prio = (CONFIG_NUM_PREEMPT_PRIORITIES - z_prio - 1); + __ASSERT_NO_MSG(prio < CONFIG_NUM_PREEMPT_PRIORITIES); } return prio; @@ -68,9 +68,11 @@ static int32_t posix_to_zephyr_priority(uint32_t priority, int policy) if (policy == SCHED_FIFO) { /* Zephyr COOP priority starts from -1 */ + __ASSERT_NO_MSG(priority < CONFIG_NUM_COOP_PRIORITIES); prio = -1 * (priority + 1); } else { - prio = (CONFIG_NUM_PREEMPT_PRIORITIES - priority); + __ASSERT_NO_MSG(priority < CONFIG_NUM_PREEMPT_PRIORITIES); + prio = (CONFIG_NUM_PREEMPT_PRIORITIES - priority - 1); } return prio; diff --git a/lib/posix/pthread_sched.c b/lib/posix/pthread_sched.c index 8c1e0a076653..f23b224179fc 100644 --- a/lib/posix/pthread_sched.c +++ b/lib/posix/pthread_sched.c @@ -7,13 +7,9 @@ #include #include -static bool valid_posix_policy(int policy) +static inline bool valid_posix_policy(int policy) { - if (policy != SCHED_FIFO && policy != SCHED_RR) { - return false; - } - - return true; + return policy == SCHED_FIFO || policy == SCHED_RR; } /** @@ -23,25 +19,12 @@ static bool valid_posix_policy(int policy) */ int sched_get_priority_min(int policy) { - if (valid_posix_policy(policy) == false) { + if (!valid_posix_policy(policy)) { errno = EINVAL; return -1; } - if (IS_ENABLED(CONFIG_COOP_ENABLED)) { - if (policy == SCHED_FIFO) { - return 0; - } - } - - if (IS_ENABLED(CONFIG_PREEMPT_ENABLED)) { - if (policy == SCHED_RR) { - return 0; - } - } - - errno = EINVAL; - return -1; + return 0; } /** @@ -51,25 +34,10 @@ int sched_get_priority_min(int policy) */ int sched_get_priority_max(int policy) { - if (valid_posix_policy(policy) == false) { - errno = EINVAL; - return -1; - } - - if (IS_ENABLED(CONFIG_COOP_ENABLED)) { - if (policy == SCHED_FIFO) { - /* Posix COOP priority starts from 0 - * whereas zephyr starts from -1 - */ - return (CONFIG_NUM_COOP_PRIORITIES - 1); - } - - } - - if (IS_ENABLED(CONFIG_PREEMPT_ENABLED)) { - if (policy == SCHED_RR) { - return CONFIG_NUM_PREEMPT_PRIORITIES; - } + if (IS_ENABLED(CONFIG_COOP_ENABLED) && policy == SCHED_FIFO) { + return CONFIG_NUM_COOP_PRIORITIES - 1; + } else if (IS_ENABLED(CONFIG_PREEMPT_ENABLED) && policy == SCHED_RR) { + return CONFIG_NUM_PREEMPT_PRIORITIES - 1; } errno = EINVAL; From 347de78981efd1588481a8e209172a75878a7dc2 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Sun, 23 Apr 2023 22:33:49 -0400 Subject: [PATCH 2/2] tests: posix: ensure that min and max priority are schedulable Verify that threads are actually schedulable for min and max scheduler priority for both `SCHED_RR` (preemptive) and `SCHED_FIFO` (cooperative). Fixes #56729 Signed-off-by: Chris Friedt (cherry picked from commit ad71b78770a446eab33a422a99bd78924dfe4f73) --- tests/posix/common/src/main.c | 4 +- tests/posix/common/src/pthread.c | 129 +++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 1 deletion(-) diff --git a/tests/posix/common/src/main.c b/tests/posix/common/src/main.c index d06a8362defb..a6f6681fb34b 100644 --- a/tests/posix/common/src/main.c +++ b/tests/posix/common/src/main.c @@ -39,6 +39,7 @@ extern void test_nanosleep_1_1(void); extern void test_nanosleep_1_1001(void); extern void test_sleep(void); extern void test_usleep(void); +extern void test_sched_policy(void); void test_main(void) { @@ -73,7 +74,8 @@ void test_main(void) ztest_unit_test(test_nanosleep_1_1001), ztest_unit_test(test_posix_pthread_create_negative), ztest_unit_test(test_sleep), - ztest_unit_test(test_usleep) + ztest_unit_test(test_usleep), + ztest_unit_test(test_sched_policy) ); ztest_run_test_suite(posix_apis); } diff --git a/tests/posix/common/src/pthread.c b/tests/posix/common/src/pthread.c index dea938a98a55..93d3222516e8 100644 --- a/tests/posix/common/src/pthread.c +++ b/tests/posix/common/src/pthread.c @@ -577,3 +577,132 @@ void test_pthread_descriptor_leak(void) zassert_ok(pthread_join(pthread1, &unused), "unable to join thread %zu", i); } } + +void test_sched_policy(void) +{ + /* + * TODO: + * 1. assert that _POSIX_PRIORITY_SCHEDULING is defined + * 2. if _POSIX_SPORADIC_SERVER or _POSIX_THREAD_SPORADIC_SERVER are defined, + * also check SCHED_SPORADIC + * 3. SCHED_OTHER is mandatory (but may be equivalent to SCHED_FIFO or SCHED_RR, + * and is implementation defined) + */ + + int pmin; + int pmax; + pthread_t th; + pthread_attr_t attr; + struct sched_param param; + static const int policies[] = { + SCHED_FIFO, + SCHED_RR, + SCHED_INVALID, + }; + static const char *const policy_names[] = { + "SCHED_FIFO", + "SCHED_RR", + "SCHED_INVALID", + }; + static const bool policy_enabled[] = { + IS_ENABLED(CONFIG_COOP_ENABLED), + IS_ENABLED(CONFIG_PREEMPT_ENABLED), + false, + }; + static int nprio[] = { + CONFIG_NUM_COOP_PRIORITIES, + CONFIG_NUM_PREEMPT_PRIORITIES, + 42, + }; + const char *const prios[] = {"pmin", "pmax"}; + + BUILD_ASSERT(!(SCHED_INVALID == SCHED_FIFO || SCHED_INVALID == SCHED_RR), + "SCHED_INVALID is itself invalid"); + + for (int policy = 0; policy < ARRAY_SIZE(policies); ++policy) { + if (!policy_enabled[policy]) { + /* test degenerate cases */ + errno = 0; + zassert_equal(-1, sched_get_priority_min(policies[policy]), + "expected sched_get_priority_min(%s) to fail", + policy_names[policy]); + zassert_equal(EINVAL, errno, "sched_get_priority_min(%s) did not set errno", + policy_names[policy]); + + errno = 0; + zassert_equal(-1, sched_get_priority_max(policies[policy]), + "expected sched_get_priority_max(%s) to fail", + policy_names[policy]); + zassert_equal(EINVAL, errno, "sched_get_priority_max(%s) did not set errno", + policy_names[policy]); + continue; + } + + /* get pmin and pmax for policies[policy] */ + for (int i = 0; i < 2; ++i) { + errno = 0; + if (i == 0) { + pmin = sched_get_priority_min(policies[policy]); + param.sched_priority = pmin; + } else { + pmax = sched_get_priority_max(policies[policy]); + param.sched_priority = pmax; + } + + zassert_not_equal(-1, param.sched_priority, + "sched_get_priority_%s(%s) failed: %d", + i == 0 ? "min" : "max", policy_names[policy], errno); + zassert_equal(0, errno, "sched_get_priority_%s(%s) set errno to %s", + i == 0 ? "min" : "max", policy_names[policy], errno); + } + + /* + * IEEE 1003.1-2008 Section 2.8.4 + * conforming implementations should provide a range of at least 32 priorities + * + * Note: we relax this requirement + */ + zassert_true(pmax > pmin, "pmax (%d) <= pmin (%d)", pmax, pmin, + "%s min/max inconsistency: pmin: %d pmax: %d", policy_names[policy], + pmin, pmax); + + /* + * Getting into the weeds a bit (i.e. whitebox testing), Zephyr + * cooperative threads use [-CONFIG_NUM_COOP_PRIORITIES,-1] and + * preemptive threads use [0, CONFIG_NUM_PREEMPT_PRIORITIES - 1], + * where the more negative thread has the higher priority. Since we + * cannot map those directly (a return value of -1 indicates error), + * we simply map those to the positive space. + */ + zassert_equal(pmin, 0, "unexpected pmin for %s", policy_names[policy]); + zassert_equal(pmax, nprio[policy] - 1, "unexpected pmax for %s", + policy_names[policy]); /* test happy paths */ + + for (int i = 0; i < 2; ++i) { + /* create threads with min and max priority levels */ + zassert_equal(0, pthread_attr_init(&attr), + "pthread_attr_init() failed for %s (%d) of %s", prios[i], + param.sched_priority, policy_names[policy]); + + zassert_equal(0, pthread_attr_setschedpolicy(&attr, policies[policy]), + "pthread_attr_setschedpolicy() failed for %s (%d) of %s", + prios[i], param.sched_priority, policy_names[policy]); + + zassert_equal(0, pthread_attr_setschedparam(&attr, ¶m), + "pthread_attr_setschedparam() failed for %s (%d) of %s", + prios[i], param.sched_priority, policy_names[policy]); + + zassert_equal(0, pthread_attr_setstack(&attr, &stack_e[0][0], STACKS), + "pthread_attr_setstack() failed for %s (%d) of %s", prios[i], + param.sched_priority, policy_names[policy]); + + zassert_equal(0, pthread_create(&th, &attr, create_thread1, NULL), + "pthread_create() failed for %s (%d) of %s", prios[i], + param.sched_priority, policy_names[policy]); + + zassert_equal(0, pthread_join(th, NULL), + "pthread_join() failed for %s (%d) of %s", prios[i], + param.sched_priority, policy_names[policy]); + } + } +}