diff --git a/miosix/kernel/pthread.cpp b/miosix/kernel/pthread.cpp index e86836e2dd8f4041f94ea01298b68d457dbc45bc..ed8382501538c941e031d80859d9c8370f6f56da 100644 --- a/miosix/kernel/pthread.cpp +++ b/miosix/kernel/pthread.cpp @@ -331,14 +331,46 @@ int pthread_cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex, const s int pthread_cond_signal(pthread_cond_t *cond) { auto *impl=reinterpret_cast<ConditionVariable*>(cond); - impl->signal(); + bool hppw=impl->doSignal(); + (void)hppw; + /* + * A note on the conditional yield. Doing a pthread_cond_signal/broadcast is + * permitted either with the mutex locked or not. If we're calling signal + * with the mutex locked, yielding isn't a good idea even if we woke up a + * higher priority thread as we "bounce back" since the woken thread will + * block trying to lock the mutex we're holding. Also, the mutex unlock + * will yield anyway and immediately switch to the higher priority thread. + * The issue is, within signal/broadcast, we don't know if we're being + * called with the mutex locked or not. + * So, we implement the following design choice: + * - in the ConditionVariable class we unconditionally yield. Since + * ConditionVariable can also be used with priority inheritance mutexes, + * we assume that we really care about minimizing latency in waking up + * higher priority threads, and we take the bounce back overhead if the + * signal was called with the mutex locked. + * - in pthread_cond_singal/broadcast we don't yield since they can only be + * used with non-priority-inheritance-mutexes, and if the signal is + * called with no mutex locked, the actual context switch to the higher + * priority thread will be delayed to the next preemption. Except with + * EDF though, as it does not preempt after a time quantum so the delay + * could be indefinitely long. In that case, we always yield. + */ + #ifdef SCHED_TYPE_EDF + //If the woken thread has higher priority than our priority, yield + if(hppw) Thread::yield(); + #endif //SCHED_TYPE_EDF return 0; } int pthread_cond_broadcast(pthread_cond_t *cond) { auto *impl=reinterpret_cast<ConditionVariable*>(cond); - impl->broadcast(); + bool hppw=impl->doBroadcast(); + (void)hppw; + #ifdef SCHED_TYPE_EDF + //If at least one woken thread has higher priority than our priority, yield + if(hppw) Thread::yield(); + #endif //SCHED_TYPE_EDF return 0; } diff --git a/miosix/kernel/sync.cpp b/miosix/kernel/sync.cpp index e7d4541e7afaf1202a78591c2dee70b25be0723f..2a74c7ba4adba89785445c8a5994644aae58690d 100644 --- a/miosix/kernel/sync.cpp +++ b/miosix/kernel/sync.cpp @@ -478,7 +478,7 @@ TimedWaitResult ConditionVariable::timedWait(pthread_mutex_t *m, long long absTi return removed ? TimedWaitResult::Timeout : TimedWaitResult::NoTimeout; } -void ConditionVariable::signal() +bool ConditionVariable::doSignal() { bool hppw=false; { @@ -486,7 +486,7 @@ void ConditionVariable::signal() //that can only be called with irq disabled, othrwise we would use //PauseKernelLock FastInterruptDisableLock lock; - if(condList.empty()) return; + if(condList.empty()) return false; //Remove from list and wakeup Thread *t=condList.front()->thread; condList.pop_front(); @@ -496,11 +496,10 @@ void ConditionVariable::signal() if(t->IRQgetPriority()>Thread::IRQgetCurrentThread()->IRQgetPriority()) hppw=true; } - //If the woken thread has higher priority than our priority, yield - if(hppw) Thread::yield(); + return hppw; } -void ConditionVariable::broadcast() +bool ConditionVariable::doBroadcast() { bool hppw=false; { @@ -520,9 +519,7 @@ void ConditionVariable::broadcast() hppw=true; } } - //If at least one of the woken thread has higher priority than our priority, - //yield - if(hppw) Thread::yield(); + return hppw; } // diff --git a/miosix/kernel/sync.h b/miosix/kernel/sync.h index 4ba757251753296fc23af580eb56aeeae40ee1b5..9ee9a5f0776e8673cf99cfda954616ad1600de87 100644 --- a/miosix/kernel/sync.h +++ b/miosix/kernel/sync.h @@ -515,20 +515,43 @@ public: * Wakeup one waiting thread. * Currently implemented policy is fifo. */ - void signal(); + void signal() + { + //If the woken thread has higher priority than our priority, yield + if(doSignal()) Thread::yield(); + } /** * Wakeup all waiting threads. */ - void broadcast(); + void broadcast() + { + //If at least one woken thread has higher priority than our priority, yield + if(doBroadcast()) Thread::yield(); + } private: + /** + * Wakeup one waiting thread. + * Currently implemented policy is fifo. + * \return true if the woken thread has higher priority than the current one + */ + bool doSignal(); + + /** + * Wakeup all waiting threads. + * \return true if at least one of the woken threads has higher priority + * than the current one + */ + bool doBroadcast(); + //Unwanted methods - ConditionVariable(const ConditionVariable&); - ConditionVariable& operator= (const ConditionVariable&); + ConditionVariable(const ConditionVariable&) = delete; + ConditionVariable& operator= (const ConditionVariable&) = delete; - //Needs direct access to condList to check if it is empty - friend int ::pthread_cond_destroy(pthread_cond_t *cond); + friend int ::pthread_cond_destroy(pthread_cond_t *); //Needs condList + friend int ::pthread_cond_signal(pthread_cond_t *); //Needs doSignal() + friend int ::pthread_cond_broadcast(pthread_cond_t *); //Needs doBroadcast() //Memory layout must be kept in sync with pthread_cond, see pthread.cpp IntrusiveList<WaitToken> condList;