core/msg.c: possibly yield after thread_flags_wake() in queue_msg()

[kaspar030]

Contribution description

Without this fix, it is possible that a thread got set to "PENDING"
after being woken up by a message flag, but no scheduling gets
triggered. So do that if a thread got woken up.

Spotted by @jia200x.

The proposed fix triggers a redundant "thread_yield_higher()" if msg_send_reply() is used and the receiving thread is waiting for the message flag. That should only hurt minimally performance wise.

The fix also assumes that calling thread_yield_higher() is safe from ISR.

Testing procedure

Not sure we have a test case for this.

Issues/PRs references

[jia200x]

I tested this patch with #16748. It works, but I get:

RIOT/tests/gnrc_netif/bin/native/tests_gnrc_netif.elf: thread_yield_higher: interrupts are disabled - this should not be

Is it still ok?

[kaspar030]

Is it still ok?

Not really, either it is and the warning is bogus, or it points to an issue.

[fjmolinas]

@kaspar030 so does this need to wait for the behavior to be fixed for other families?

[kaspar030]

@kaspar030 so does this need to wait for the behavior to be fixed for other families?

yes, unfortunately!

[jia200x]

is this now ok, since #16754 is already merged?

[kaspar030]

is this now ok, since #16754 is already merged?

no, this would now work on native, but not some others. maybe we need a quicker fix.

[kaspar030]

quicker fix.

I think #16899 should also fix this, at the cost of an extra memory read and comparison.

[benpicco]

I think #16899 should also fix this, at the cost of an extra memory read and comparison.

Which would you rather have in?

[benpicco]

ping @kaspar030

[benpicco]

Should the same also be done in line 155?

And if the answer to this is yes, why not move the thread_yield_higher() to thread_flags_wake() since those two are the only locations where thread_flags_wake() is called.

diff --git a/core/msg.c b/core/msg.c
index 7bfb40f697..c5ffdbafff 100644
--- a/core/msg.c
+++ b/core/msg.c
@@ -55,7 +55,6 @@ static int queue_msg(thread_t *target, const msg_t *m)
 
     *dest = *m;
 #if MODULE_CORE_THREAD_FLAGS
-    target->flags |= THREAD_FLAG_MSG_WAITING;
     thread_flags_wake(target);
 #endif
     return 1;
@@ -151,7 +150,6 @@ static int _msg_send(msg_t *m, kernel_pid_t target_pid, bool block,
         thread_add_to_list(&(target->msg_waiters), me);
 
 #if MODULE_CORE_THREAD_FLAGS
-        target->flags |= THREAD_FLAG_MSG_WAITING;
         thread_flags_wake(target);
 #endif
 
diff --git a/core/thread_flags.c b/core/thread_flags.c
index 1d4db08f10..8d9e16e19b 100644
--- a/core/thread_flags.c
+++ b/core/thread_flags.c
@@ -55,7 +55,13 @@ static inline int __attribute__((always_inline)) _thread_flags_wake(
 
 int thread_flags_wake(thread_t *thread)
 {
-    return _thread_flags_wake(thread);
+    int res;
+
+    target->flags |= THREAD_FLAG_MSG_WAITING;
+    res = _thread_flags_wake(thread);
+    thread_yield_higher();
+
+    return res;
 }
 
 static thread_flags_t _thread_flags_clear_atomic(thread_t *thread,

[kaspar030]

in line 155 there's always a thread_yield following, below the irq_restore().
The yield is not in thread_flags wake because when it was implemented, thread_yield was not isr safe, not sure it is now.
Also, there's use-cases where many threads need to be woken up. maybe this needs to be revisited.

the thread_yield_higher() is frustrating, I think we need to go through all the platforms and split those. IIRC most of them do the equivalent of "if irq_is_in() sched_context_switch_request=1 else platform_stuff()", that can maybe be pulled out.

[kaspar030]

Which would you rather have in?

I think #16899, seems like the more to-the-point change.

[kaspar030]

Closing in favour of #16899.