posix: sched: ensure min and max priority are schedulable

include/zephyr/posix/sched.h

@@ -6,10 +6,19 @@
 #ifndef ZEPHYR_INCLUDE_POSIX_SCHED_H_
 #define ZEPHYR_INCLUDE_POSIX_SCHED_H_
 
+#include <zephyr/kernel.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
 
+/*
+ * Other mandatory scheduling policy. Must be numerically distinct. May
+ * execute identically to SCHED_RR or SCHED_FIFO. For Zephyr this is a
+ * pseudonym for SCHED_RR.
+ */
+#define SCHED_OTHER 0
+
 /* Cooperative scheduling policy */
 #define SCHED_FIFO 1
 

lib/posix/posix_internal.h

@@ -7,6 +7,8 @@
 #ifndef ZEPHYR_LIB_POSIX_POSIX_INTERNAL_H_
 #define ZEPHYR_LIB_POSIX_POSIX_INTERNAL_H_
 
+#include <zephyr/kernel.h>
+
 /*
  * Bit used to mark a pthread object as initialized. Initialization status is
  * verified (against internal status) in lock / unlock / destroy functions.

lib/posix/pthread.c

@@ -14,16 +14,15 @@
 #include <zephyr/sys/slist.h>
 
 #include "posix_internal.h"
+#include "pthread_sched.h"
 
 #define PTHREAD_INIT_FLAGS	PTHREAD_CANCEL_ENABLE
 #define PTHREAD_CANCELED	((void *) -1)
 
-#define LOWEST_POSIX_THREAD_PRIORITY 1
-
 K_MUTEX_DEFINE(pthread_once_lock);
 
 static const struct pthread_attr init_pthread_attrs = {
-	.priority = LOWEST_POSIX_THREAD_PRIORITY,
+	.priority = 0,
 	.stack = NULL,
 	.stacksize = 0,
 	.flags = PTHREAD_INIT_FLAGS,
@@ -56,7 +55,7 @@ struct posix_thread *to_posix_thread(pthread_t pthread)
 	return &posix_thread_pool[pthread];
 }
 
-static bool is_posix_prio_valid(uint32_t priority, int policy)
+static bool is_posix_policy_prio_valid(uint32_t priority, int policy)
 {
 	if (priority >= sched_get_priority_min(policy) &&
 	    priority <= sched_get_priority_max(policy)) {
@@ -73,9 +72,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;
@@ -87,9 +88,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;
@@ -106,7 +109,7 @@ int pthread_attr_setschedparam(pthread_attr_t *_attr, const struct sched_param *
 	int priority = schedparam->sched_priority;
 
 	if ((attr == NULL) || (attr->initialized == 0U) ||
-	    (is_posix_prio_valid(priority, attr->schedpolicy) == false)) {
+	    (is_posix_policy_prio_valid(priority, attr->schedpolicy) == false)) {
 		return EINVAL;
 	}
 
@@ -296,11 +299,11 @@ int pthread_setschedparam(pthread_t pthread, int policy,
 		return ESRCH;
 	}
 
-	if (policy != SCHED_RR && policy != SCHED_FIFO) {
+	if (!valid_posix_policy(policy)) {
 		return EINVAL;
 	}
 
-	if (is_posix_prio_valid(param->sched_priority, policy) == false) {
+	if (is_posix_policy_prio_valid(param->sched_priority, policy) == false) {
 		return EINVAL;
 	}
 
@@ -563,8 +566,7 @@ int pthread_attr_setschedpolicy(pthread_attr_t *_attr, int policy)
 {
 	struct pthread_attr *attr = (struct pthread_attr *)_attr;
 
-	if ((attr == NULL) || (attr->initialized == 0U) ||
-	    (policy != SCHED_RR && policy != SCHED_FIFO)) {
+	if ((attr == NULL) || (attr->initialized == 0U) || !valid_posix_policy(policy)) {
 		return EINVAL;
 	}
 

lib/posix/pthread_sched.c

@@ -4,44 +4,24 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+#include "pthread_sched.h"
+
 #include <zephyr/kernel.h>
 #include <zephyr/posix/sched.h>
 
-static bool valid_posix_policy(int policy)
-{
-	if (policy != SCHED_FIFO && policy != SCHED_RR) {
-		return false;
-	}
-
-	return true;
-}
-
 /**
  * @brief Get minimum priority value for a given policy
  *
  * See IEEE 1003.1
  */
 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 +31,11 @@ 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 || policy == SCHED_OTHER)) {
+		return CONFIG_NUM_PREEMPT_PRIORITIES - 1;
 	}
 
 	errno = EINVAL;

lib/posix/pthread_sched.h

@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2023 Meta
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#ifndef ZEPHYR_LIB_POSIX_POSIX_PTHREAD_SCHED_H_
+#define ZEPHYR_LIB_POSIX_POSIX_PTHREAD_SCHED_H_
+
+#include <stdbool.h>
+
+#include <zephyr/posix/sched.h>
+
+static inline bool valid_posix_policy(int policy)
+{
+	return policy == SCHED_FIFO || policy == SCHED_RR || policy == SCHED_OTHER;
+}
+
+#endif

tests/posix/common/src/pthread.c

@@ -598,3 +598,137 @@ ZTEST(posix_apis, test_pthread_descriptor_leak)
 		zassert_ok(pthread_join(pthread1, &unused), "unable to join thread %zu", i);
 	}
 }
+
+ZTEST(posix_apis, test_sched_policy)
+{
+	/*
+	 * 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_OTHER,
+		SCHED_INVALID,
+	};
+	static const char *const policy_names[] = {
+		"SCHED_FIFO",
+		"SCHED_RR",
+		"SCHED_OTHER",
+		"SCHED_INVALID",
+	};
+	static const bool policy_enabled[] = {
+		IS_ENABLED(CONFIG_COOP_ENABLED),
+		IS_ENABLED(CONFIG_PREEMPT_ENABLED),
+		IS_ENABLED(CONFIG_PREEMPT_ENABLED),
+		false,
+	};
+	static int nprio[] = {
+		CONFIG_NUM_COOP_PRIORITIES,
+		CONFIG_NUM_PREEMPT_PRIORITIES,
+		CONFIG_NUM_PREEMPT_PRIORITIES,
+		42,
+	};
+	const char *const prios[] = {"pmin", "pmax"};
+
+	BUILD_ASSERT(!(SCHED_INVALID == SCHED_FIFO || SCHED_INVALID == SCHED_RR ||
+		       SCHED_INVALID == SCHED_OTHER),
+		     "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_ok(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_ok(pthread_attr_init(&attr),
+				   "pthread_attr_init() failed for %s (%d) of %s", prios[i],
+				   param.sched_priority, policy_names[policy]);
+
+			zassert_ok(pthread_attr_setschedpolicy(&attr, policies[policy]),
+				   "pthread_attr_setschedpolicy() failed for %s (%d) of %s",
+				   prios[i], param.sched_priority, policy_names[policy]);
+
+			zassert_ok(pthread_attr_setschedparam(&attr, &param),
+				   "pthread_attr_setschedparam() failed for %s (%d) of %s",
+				   prios[i], param.sched_priority, policy_names[policy]);
+
+			zassert_ok(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_ok(pthread_create(&th, &attr, create_thread1, NULL),
+				   "pthread_create() failed for %s (%d) of %s", prios[i],
+				   param.sched_priority, policy_names[policy]);
+
+			zassert_ok(pthread_join(th, NULL),
+				   "pthread_join() failed for %s (%d) of %s", prios[i],
+				   param.sched_priority, policy_names[policy]);
+		}
+	}
+}