core/sched_round_robin: Add a simple round robin scheduler to core by kfessel · Pull Request #15069 · RIOT-OS/RIOT

kfessel · 2020-09-23T13:24:40Z

includes example
USEMODULE activate round robin for all not masked priorities

inspired by the comments on #14810 i created a core variant of a round robin scheduler

Contribution description

This contains a simple round robin for the core scheduler using xtimer to rearrange the run queue.

nothing special is done about fainess to keep this low in memory consumption.

Testing procedure

compile the example either with or without USEMODULE += sched_round_robin

without this module only one of the threads will run.
with this module they run parallel , sharing cpu time.

Issues/PRs references

similar but non core: #14810

kfessel · 2020-09-23T13:26:19Z

maybe @maribu and @benpicco want to have a look at this since they where the most active reviewers on #14810

maribu

Some inline comments. I didn't look into the concepts or implementation details yet. Feel free to directly squash in all the style issues.

maribu · 2020-09-23T13:44:10Z

core/include/sched_round_robin.h

+/**
+ * @brief masks off priorities that should not be scheduled default: 0 is masked
+ */
+#define SCHED_RR_MASK 1<<0


Suggested change

#define SCHED_RR_MASK 1<<0

#define SCHED_RR_MASK (1 << 0)

maribu · 2020-09-23T13:45:16Z

core/include/sched_round_robin.h

+/**
+ * @brief setup the round robin timer for prio
+ */
+void set_sched_round_robin(uint8_t prio);


Missing documentation for parameter prio.

It would be nice if all functions would contain a common prefix. E.g. sched_rr_...

i will switch the function names around from "set_sched_round_robin" to "sched_round_robin_set"
since the is no need to keep set_.. public i will be static inline

maribu · 2020-09-23T13:45:58Z

core/sched_round_robin.c

+    if (current_rr_priority != 0xff) {
+        DEBUG_PUTS("Round_Robin_cb");
+        clist_lpoprpush(&sched_runqueues[current_rr_priority]);
+        thread_t * active_thread = thread_get_active();


Suggested change

thread_t * active_thread = thread_get_active();

thread_t *active_thread = thread_get_active();

i still not understand why you like to put the type close to the name and have no space between type and name

maribu · 2020-09-23T13:46:12Z

core/sched_round_robin.c

+        DEBUG_PUTS("Round_Robin_cb");
+        clist_lpoprpush(&sched_runqueues[current_rr_priority]);
+        thread_t * active_thread = thread_get_active();
+        if(active_thread){


Suggested change

if(active_thread){

if (active_thread) {

maribu · 2020-09-23T13:46:26Z

core/sched_round_robin.c

+        thread_t * active_thread = thread_get_active();
+        if(active_thread){
+            uint8_t active_priority = active_thread->priority;
+            if(active_priority == current_rr_priority){


Suggested change

if(active_priority == current_rr_priority){

if (active_priority == current_rr_priority) {

maribu · 2020-09-23T13:49:37Z

core/sched_round_robin.c

+    if(current_rr_priority == prio){
+        remove_sched_round_robin();
+        thread_t * active_thread = thread_get_active();
+        if(active_thread){


Suggested change

if(active_thread){

if (active_thread) {

maribu · 2020-09-23T13:50:37Z

examples/thread-duell/main.c

+}
+
+
+void * thread_bad(void * d)


Suggested change

void * thread_bad(void * d)

void * thread_bad(void *d)

maribu · 2020-09-23T13:50:48Z

examples/thread-duell/main.c

+{
+    xtimer_usleep64(200 * 1000);  /*allways be nice at start*/
+#ifdef DEVELHELP
+    const char * name = thread_get_active()->name;


Suggested change

const char * name = thread_get_active()->name;

const char *name = thread_get_active()->name;

maribu · 2020-09-23T13:51:06Z

examples/thread-duell/main.c

+#endif
+
+    uint32_t w = 0;
+    uint32_t work = * (uint32_t *) d;


Suggested change

uint32_t work = * (uint32_t *) d;

uint32_t work = *((uint32_t *)d);

maribu · 2020-09-23T13:57:49Z

core/include/sched_round_robin.h

+/**
+ * @brief setup round robin for priority if possible an sensible
+ */
+void check_set_sched_round_robin(uint8_t prio);


From the documentation it is not clear what the difference is to set_sched_round_robin(). Do we really need two functions?

IMO we should try not to provide largely overlapping functions to the public. It will confuse users (they have to decide on a specific version of a function-family) and it increases the maintenance burden.

(Note: I'm certainly a huge fan of many small internal functions that help de-duplicating and structuring code. My comment above only applies for the public APIs.)

you are right i already thought about making set_sched_round_robin() static an suggest inlineing to the compiler

includes example USEMODULE activate round robin for all not masked prioritys

kfessel · 2020-09-23T16:25:53Z

added style changes and squashed

fjmolinas · 2020-09-24T08:56:47Z

I'm personally against adding a dependency to xtimer in core.

fjmolinas · 2020-09-24T08:58:45Z

I'm personally against adding a dependency to xtimer in core.

I know this is why @kfessel had included this functionality in sys in the initial PR, so I agree with his initial thought.

maribu · 2020-09-24T09:05:45Z

I'm personally against adding a dependency to xtimer in core.

I know this is why @kfessel had included this functionality in sys in the initial PR, so I agree with his initial thought.

How about extending core/sched with the required round robin primitives. An implementation in sys could then use this public API in conjunction with xtimer to implement round robin scheduling.

I'm personally strongly against code outside of core (and to some degree cpu) touching any internals in core. This had led to quite some pain before.

kfessel · 2020-09-24T12:21:08Z

It would be very difficult to split this thing right at the calls to xtimer the result would be a xtimer like interface on top of it wich would also have to reside in sys since it uses xtimer and would be called from core since there is the RR logic, one could add something like shed_cb to call in case of any timer function work which would make implementing scheduler modules in sys possible but this would also require the sys to manipulate the core structures like sys/sched_rr did; we could extend the interfaces to the runqueues in core and use them from sys. which would mean it is either very specific to RR (eg provide a function that switches to the next task in a runqueue) or very inefficient (maybe both at the same time)

this core variant has some advantages over the sys version (not running when it is not needed) through to deeper integration <- i like that;
the sys variant has asside from manipulating core structures very low impact on the rest of the code <- i like that;
I am hassitant to do a deep interating thing which is than keept in sys.
We could instead of deepending on xtimer depend on a generic timer implementation which we keep in core like some systick an decide on the hardware base which implementation to take. i think such a thing could be a littlebit simpler than x/ztimer but i dont think i would be much.

kfessel · 2020-09-25T15:14:34Z

There some features in core that depend on sys-modules being present they are activated when the sys-module is configured: auto_init (depends on sys/auto-init) and panic-ps-printing (depends on sys/ps) and panic usb-board-reset (depends on sys/usb_board_reset), ps also reads internal core structures, and board reset will certainly change them (considdering this i could move the core variant to sys (but i don't think this wold be more readable).
For statistics there is a real clean single purpose interface created for one sys-module (which can be replaced by another user)

I know this i quiet a stretch to argument that there are already dependancies of core on sys, but i can also add a && MODULE_XTIMER to the if defs deactivating the feature if no xtimer is present making it just a configurable core feature that only get activated when xtimer is present (like panic-ps).

There is another example a very hard dependency from core to sys: libc is considered sys as far as i can see and it is used by core not only a single function but also many types, this dependency is never questioned (no #ifdef) it is always a given that there is a libc in sys and core uses it.

kfessel · 2021-03-17T09:27:36Z

Closing this in favor of #16126 please continue the review there

kfessel requested review from jia200x and kaspar030 as code owners September 23, 2020 13:24

maribu requested review from benpicco and maribu September 23, 2020 13:42

maribu reviewed Sep 23, 2020

View reviewed changes

core/sched_round_robin: Add a simple round robin scheduler to core

1604950

includes example USEMODULE activate round robin for all not masked prioritys

kfessel force-pushed the sched_rr_core branch from fe93545 to 1604950 Compare September 23, 2020 16:24

kfessel mentioned this pull request Feb 12, 2021

RIOT-core is missing timer and time functionality #15080

Closed

kfessel mentioned this pull request Mar 2, 2021

sys/sched_rr: Add a round robin scheduler module #16126

Merged

kfessel closed this Mar 17, 2021

	thread_t * active_thread = thread_get_active();
	thread_t *active_thread = thread_get_active();

	if(active_priority == current_rr_priority){
	if (active_priority == current_rr_priority) {

	const char * name = thread_get_active()->name;
	const char *name = thread_get_active()->name;

	uint32_t work = * (uint32_t *) d;
	uint32_t work = ((uint32_t )d);

Conversation

kfessel commented Sep 23, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kfessel commented Sep 23, 2020

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel commented Sep 23, 2020

Uh oh!

fjmolinas commented Sep 24, 2020

Uh oh!

fjmolinas commented Sep 24, 2020

Uh oh!

maribu commented Sep 24, 2020

Uh oh!

kfessel commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfessel commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfessel commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kfessel commented Sep 24, 2020 •

edited

Loading

kfessel commented Sep 25, 2020 •

edited

Loading