core/sched.c: fix _runqueue_pop() removing wrong thread

[derMihai]

Contribution description

_runqueue_pop() calls clist_lpop(), which pops the leftmost entry in the thread's priority runqueue, but not necessarily the one passed as argument.

I understand the original logic behind this: if a thread is to be removed, then it is so because it is about to block on something or exit. And a thread can only block or exit if it's currently running. As a result, it must be the leftmost entry on the runqueue. But this isn't always true. Consider sched_change_priority(), which does the following:

    if (thread_is_active(thread)) { 
        _runqueue_pop(thread);
        _runqueue_push(thread, priority);
    }

thread_is_active() returns true if a thread is either running or queued to be running. So the _runqueue_pop() might retrieve any thread. Let's extend _runqueue_pop() with the following assertion:

    ...
    clist_node_t *removed = clist_lpop(&sched_runqueues[thread->priority]);
    assume(container_of(removed, thread_t, rq_entry) == thread);
    ...

Running this will trigger the assertion:

    int thread_ids[WAITERS_CNT];
    for (unsigned i = 0; i < WAITERS_CNT; i++) {
       // set a lower priority than the current thread's to make sure they are all in the runqueue when
       // changing priorities below
        thread_ids[i] = thread_create(stacks[i], sizeof(stacks[0]), THREAD_PRIORITY_MAIN + 1,
                      THREAD_CREATE_STACKTEST, waiter, NULL, "waiter");
    }

    thread_t *thread2 = thread_get(thread_ids[1]);

    irq_disable(); // sched_change_priority() wants interrupts disabled
    sched_change_priority(thread2, THREAD_PRIORITY_MAIN);
    irq_enable();

If we change _runqueue_pop() to search for the specific thread:

    ...
    clist_node_t *removed = clist_remove(&sched_runqueues[thread->priority], &thread->rq_entry);
    assume(container_of(removed, thread_t, rq_entry) == thread);
    ....

Then everything is all right.

The only ugly thing is that, when the thread is indeed running (vast majority of cases), due to how clists are implemented the whole list is walked to remove the thread.

Testing procedure

I briefly tested this code on a SAM0 board.

[riot-ci]

Murdock results

✔️ PASSED

ba71ba7 core/sched.c: fix _runqueue_pop() removing wrong thread

Success	Failures	Total	Runtime
10251	0	10251	19m:18s

Artifacts

• Documentation preview

[benpicco]

The only ugly thing is that, when the thread is indeed running (vast majority of cases), due to how clists are implemented the whole list is walked to remove the thread.

clist_remove() already contains an optimisation there:

static inline clist_node_t *clist_remove(clist_node_t *list, clist_node_t *node)
{
    if (list->next) {
        if (list->next->next == node) {
            return clist_lpop(list);
        }
        else {

So in the common case, this just adds two if conditions.

[derMihai]

So in the common case, this just adds two if conditions

If the common case is that there are exactly one or two threads, and we want to remove the first. If there are more, it will start looping from the second. But probably good enough.

[benpicco]

looks like Makefile.ci needs an update - can you check by how much this increases code size?

It's probably just a few bytes and tests_xbee was already at the very limit for nucleo-l031k6

[derMihai]

looks like Makefile.ci needs an update - can you check by how much this increases code size?

It's probably just a few bytes and tests_xbee was already at the very limit for nucleo-l031k6

seems to be working again.

[benpicco]

No, CI still failed

[derMihai]

make BOARD=nucleo-l031k6 for tests/drivers/xbee fails on master too:

/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: region `rom' overflowed by 3596 bytes

on my branch:

/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: region `rom' overflowed by 3680 bytes

So a code-size increase by 84 bytes. I suppose this happens because clist_remove() is very rarely used and most probably not in this application (grep returns a few results, but hard to tell for sure as neither master nor my branch produce a binary), but now it's in.

Also, why is All checks have passed active if some build failed?

[maribu]

With the toolchain used in the CI the binary get's smaller. The increase by 84 B was enough to tip it over the limit, as it is now 8 B larger than flash is available.

That is fine. We want things to be correct first and small/efficient second. So just add the board to the list in the Makefile.ci to record it is no longer expected to be linkable with the CI toolchain, and we can merge this. (Note: You can use make generate-Makefile.ci to build it.)