posix: pthread: Clarify SCHED_OTHER scheduling policy #67017

golowanow · 2023-12-26T20:03:19Z

The following scheduling scenario is now reflected as POSIX policy SCHED_OTHER:
when Zephyr Kconfig allows both cooperative and preemptive thread scheduling classes with application thread priority ranges either (-CONFIG_NUM_COOP_PRIORITIES..-1) or (0..CONFIG_NUM_PREEMPT_PRIORITIES-1) respectively highest to lowest, translated to POSIX thread priorities in range (0..CONFIG_NUM_PREEMPT_PRIORITIES+CONFIG_NUM_COOP_PRIORITIES-1) lowest to highest.

predecessor: #57161

The following scheduling scenario is now reflected as POSIX policy SCHED_OTHER: when Zephyr Kconfig allows both cooperative and preemptive thread scheduling classes with application thread priority ranges either (-CONFIG_NUM_COOP_PRIORITIES..-1) or (0..CONFIG_NUM_PREEMPT_PRIORITIES-1) respectively highest to lowest, translated to POSIX thread priorities in range (0..CONFIG_NUM_PREEMPT_PRIORITIES+CONFIG_NUM_COOP_PRIORITIES-1) lowest to highest. Signed-off-by: Dmitrii Golovanov <[email protected]>

cfriedt · 2024-01-03T17:28:02Z

I'm going to take a bit of time to review this because I had some minor concerns and want to ensure that I look at all of the various angles. Thanks for the PR though 👍

tests/posix/common/src/pthread.c

cfriedt · 2024-01-08T00:42:00Z

tests/posix/common/src/pthread.c

-	static int nprio[] = {
-		CONFIG_NUM_COOP_PRIORITIES,
-		CONFIG_NUM_PREEMPT_PRIORITIES,


Is it possible to still keep nprio[]? From what I can tell, only the third value (2, with zero indexing) would need to change.

CONFIG_NUM_COOP_PRIORITIES + CONFIG_NUM_PREEMPT_PRIORITIES looks good to me, which is (I think) where you are going.

I just noticed that nprio[] is also not const, but it probably should be. Feel free to add that drive-by fix, if you're so inclined. Maybe there was a reason not to make it const before (possible compile error?)

I think it causes a compile error if it's const, but I'm hoping we can still keep it 🤷

cfriedt · 2024-01-08T02:42:19Z

tests/posix/common/src/pthread.c

+	 * preemptive threads use [0, CONFIG_NUM_PREEMPT_PRIORITIES - 1],
+	 * where the more negative thread has the higher priority.
+	 */
+	static int prio_min[] = {


I believe we do not actually need prio_min[] or prio_max[] and it's possible that they make the problem more confusing.

POSIX thread priorities grow up from 0, while Zephyr thread priorities grow increasingly negative.

If it helps, I could be more specific about it, so please let me know and we can chat on Discord.

cfriedt · 2024-01-08T02:42:30Z

tests/posix/common/src/pthread.c

+		0,
+		-1,
+	};
+	static int prio_max[] = {


I believe we do not actually need prio_min[] or prio_max[] and it's possible that they make the problem more confusing.

POSIX thread priorities grow up from 0, while Zephyr thread priorities grow increasingly negative.

If it helps, I could be more specific about it, so please let me know and we can chat on Discord.

cfriedt · 2024-01-08T03:00:13Z

lib/posix/sched.c

-	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;
+	int priority_levels = 0;
+
+	if (!valid_posix_policy(policy)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (IS_ENABLED(CONFIG_PREEMPT_ENABLED)) {
+		priority_levels += CONFIG_NUM_PREEMPT_PRIORITIES;
+	}
+
+	if (IS_ENABLED(CONFIG_COOP_ENABLED) && policy != SCHED_RR) {
+		priority_levels += CONFIG_NUM_COOP_PRIORITIES;
+	}
+
+	if (priority_levels == 0) {
+		/* Should be already eliminated at compile time. */
+		errno = EINVAL;
+		return -1;
 	}

-	errno = EINVAL;
-	return -1;
+	return priority_levels - 1;    /* 0 inclusive */


I think this can be even simpler (i.e. no runtime arithmetic)

Suggested change

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;

int priority_levels = 0;

if (!valid_posix_policy(policy)) {

errno = EINVAL;

return -1;

}

if (IS_ENABLED(CONFIG_PREEMPT_ENABLED)) {

priority_levels += CONFIG_NUM_PREEMPT_PRIORITIES;

}

if (IS_ENABLED(CONFIG_COOP_ENABLED) && policy != SCHED_RR) {

priority_levels += CONFIG_NUM_COOP_PRIORITIES;

}

if (priority_levels == 0) {

/* Should be already eliminated at compile time. */

errno = EINVAL;

return -1;

}

errno = EINVAL;

return -1;

return priority_levels - 1; /* 0 inclusive */

if (CONFIG_NUM_COOP_PRIORITIES > 0 && policy == SCHED_FIFO) {

return CONFIG_NUM_COOP_PRIORITIES - 1;

} else if (CONFIG_NUM_PREEMPT_PRIORITIES > 0 &&

(policy == SCHED_RR || policy == SCHED_OTHER)) {

return CONFIG_NUM_PREEMPT_PRIORITIES - 1;

} else if (CONFIG_NUM_COOP_PRIORITIES > 0 && CONFIG_NUM_PREEMPT_PRIORITIES > 0 &&

policy == SCHED_OTHER) {

return CONFIG_NUM_PREEMPT_PRIORITIES - 1;

}

errno = EINVAL;

return -1;

cfriedt · 2024-01-08T03:07:54Z

lib/posix/sched.c

+ *
+ * @param[in] policy  Scheduling policy with Zephyr thread priority ranges:
+ *	SCHED_RR:   [0, @ref CONFIG_NUM_PREEMPT_PRIORITIES -1]
+ *	SCHED_FIFO: [ @ref CONFIG_NUM_PREEMPT_PRIORITIES,
+ *		      @ref CONFIG_NUM_COOP_PRIORITIES -1]
+ *	SCHED_OTHER: [0, @ref CONFIG_NUM_PREEMPT_PRIORITIES +
+ *			 @ref CONFIG_NUM_COOP_PRIORITIES -1]
+ *
+ * @retval -1 and errno=EINVAL for a policy which is either not supported
+ *	or not allowed with Zephyr config parameters at compile time.


The SCHED FIFO bit is incorrect. It's possible that the current positive-defined mapping for posix priorities is not very clear though, please see other comments.

And actually, there is no real need to doxygenate any of the internal API calls here. Their output is not processed in terms of Zephyr API pages. I think we will likely need to doxygenate headers, and IMHO, it would make more sense to do that later, all at once, using some kind of system, because there are a lot of things to doxygenate, (if that's even the route we go to generate POSIX docs).

You might as well leave this one as-is.

cfriedt · 2024-01-08T03:08:35Z

lib/posix/sched.c

+ *
+ * @param[in] policy  Scheduling policy with Zephyr thread priority ranges:
+ *	SCHED_RR:   [0, @ref CONFIG_NUM_PREEMPT_PRIORITIES -1]
+ *	SCHED_FIFO: [ @ref CONFIG_NUM_PREEMPT_PRIORITIES,
+ *		      @ref CONFIG_NUM_COOP_PRIORITIES -1]
+ *	SCHED_OTHER: [0, @ref CONFIG_NUM_PREEMPT_PRIORITIES +
+ *			 @ref CONFIG_NUM_COOP_PRIORITIES -1]
+ *
+ * @retval -1 and errno=EINVAL for a policy which is either not supported
+ *	or not allowed with Zephyr config parameters at compile time.


The SCHED FIFO bit is incorrect. It's possible that the current positive-defined mapping for posix priorities is not very clear though, please see other comments.

And actually, there is no real need to doxygenate any of the internal API calls here. Their output is not processed in terms of Zephyr API pages. I think we will likely need to doxygenate headers, and IMHO, it would make more sense to do that later, all at once, using some kind of system, because there are a lot of things to doxygenate, (if that's even the route we go to generate POSIX docs).

cfriedt · 2024-01-08T03:11:07Z

lib/posix/pthread.c

@@ -198,38 +198,59 @@ void __z_pthread_cleanup_pop(int execute)
 	}
 }

-static bool is_posix_policy_prio_valid(uint32_t priority, int policy)


I don't believe this function needs to change at all. Please revert. Posix priorities are (by definition) positive, since negative priorities would interfere with a return value of -1 signifying error.

That was the cause of a pretty massive bug that I fixed probably over a year ago.

I don't believe this function needs to change at all. Please revert. Posix priorities are (by definition) positive, since negative priorities would interfere with a return value of -1 signifying error.

this function's purpose is to validate POSIX priority value, including it is positive, isn't it ?
although currently it takes uint32_t which either already implies a positive value, or some 'weird' validation logic implemented.

The function is called to check user input from pthread_attr_setschedparam() and pthread_setschedparam() at struct sched_param where it is int, so making same type here to avoid implicit uint32_t type conversion and clarify the validation logic.

cfriedt · 2024-01-08T03:13:25Z

lib/posix/pthread.c

-static uint32_t zephyr_to_posix_priority(int32_t z_prio, int *policy)
+static int zephyr_to_posix_priority(int32_t z_prio, int *policy)
 {
 	if (z_prio < 0) {
-		__ASSERT_NO_MSG(z_prio < CONFIG_NUM_COOP_PRIORITIES);
+		/* COOP: highest [-CONFIG_NUM_COOP_PRIORITIES, -1] low */
+		__ASSERT_NO_MSG((CONFIG_NUM_COOP_PRIORITIES > 0) &&
+				(z_prio < -CONFIG_NUM_COOP_PRIORITIES));
+	} else {
+		/* PREEMPT: high [0, CONFIG_NUM_PREEMPT_PRIORITIES -1] lowest */
+		__ASSERT_NO_MSG((CONFIG_NUM_PREEMPT_PRIORITIES > 0) &&
+				(z_prio < CONFIG_NUM_PREEMPT_PRIORITIES));
+	}
+
+	if (IS_ENABLED(CONFIG_PREEMPT_ENABLED) &&
+	    IS_ENABLED(CONFIG_COOP_ENABLED)) {
+		*policy = SCHED_OTHER;
 	} else {
-		__ASSERT_NO_MSG(z_prio < CONFIG_NUM_PREEMPT_PRIORITIES);
+		*policy = (z_prio < 0) ? SCHED_FIFO : SCHED_RR;
 	}

-	*policy = (z_prio < 0) ? SCHED_FIFO : SCHED_RR;
-	return ZEPHYR_TO_POSIX_PRIORITY(z_prio);
+	return ZEPHYR_TO_POSIX_PRIORITY(z_prio) +
+		(IS_ENABLED(CONFIG_PREEMPT_ENABLED) ?
+			CONFIG_NUM_PREEMPT_PRIORITIES : 0);


With the additional information that POSIX priorities must be >= 0, this probably needs a bit of a rework.

Ping me on Discord if you need any 1:1

Hmm... I think this will be a lot easier to do once #67223 is done.

The problem:

without having a copy of the thread policy built-in to struct posix_thread, it's impossible to know, with confidence, whether someone using the k_thread API has modified the policy of a pthread. We can only hope, currently, that is not the case. So there is a chance that pthread and k_thread priority changes can introduce undetectable changes in scheduling policy, which is... not great.

At least with #67223 it will be possible to detect if a thread priority has been modified behind our backs.

MaureenHelm · 2024-03-07T16:05:15Z

@golowanow do you plan to return to this PR?

github-actions · 2024-05-07T00:29:42Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

golowanow force-pushed the posix_fix_20231225 branch from 455c572 to d8dd709 Compare December 26, 2023 20:12

golowanow force-pushed the posix_fix_20231225 branch from d8dd709 to 46f0008 Compare December 26, 2023 20:22

golowanow marked this pull request as ready for review December 26, 2023 21:43

zephyrbot added the area: POSIX POSIX API Library label Dec 26, 2023

zephyrbot requested a review from cfriedt December 26, 2023 21:43

zephyrbot assigned cfriedt Dec 26, 2023

golowanow mentioned this pull request Dec 27, 2023

posix: implement sched_getparam() #66959

Closed

golowanow added the Enhancement Changes/Updates/Additions to existing features label Dec 28, 2023

cfriedt reviewed Jan 8, 2024

View reviewed changes

tests/posix/common/src/pthread.c Show resolved Hide resolved

cfriedt reviewed Jan 8, 2024

View reviewed changes

tests/posix/common/src/pthread.c Show resolved Hide resolved

cfriedt reviewed Jan 8, 2024

View reviewed changes

github-actions bot added the Stale label May 7, 2024

github-actions bot closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

posix: pthread: Clarify SCHED_OTHER scheduling policy #67017

posix: pthread: Clarify SCHED_OTHER scheduling policy #67017

golowanow commented Dec 26, 2023 •

edited

Loading

cfriedt commented Jan 3, 2024

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024

cfriedt Jan 8, 2024

cfriedt Jan 8, 2024 •

edited

Loading

golowanow Jan 14, 2024

cfriedt Jan 8, 2024

cfriedt Jan 8, 2024

MaureenHelm commented Mar 7, 2024

github-actions bot commented May 7, 2024

posix: pthread: Clarify SCHED_OTHER scheduling policy #67017

posix: pthread: Clarify SCHED_OTHER scheduling policy #67017

Conversation

golowanow commented Dec 26, 2023 • edited Loading

cfriedt commented Jan 3, 2024

cfriedt Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt Jan 8, 2024

Choose a reason for hiding this comment

cfriedt Jan 8, 2024

Choose a reason for hiding this comment

cfriedt Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

golowanow Jan 14, 2024

Choose a reason for hiding this comment

cfriedt Jan 8, 2024

Choose a reason for hiding this comment

cfriedt Jan 8, 2024

Choose a reason for hiding this comment

MaureenHelm commented Mar 7, 2024

github-actions bot commented May 7, 2024

golowanow commented Dec 26, 2023 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading

cfriedt Jan 8, 2024 •

edited

Loading